Date   

Re: UEFI terminal console keyboard type extend for Putty

Liming Gao
 

Zhichao:
One clarification. What terminal type will be introduced? Xterm, VT400 and Linux?

Thanks
Liming
From: Gao, Zhichao
Sent: Friday, September 06, 2019 12:56 PM
To: rfc@edk2.groups.io
Cc: devel@edk2.groups.io; Gao, Liming <liming.gao@...>; Xu, Shiwei <shiwei.xu@...>
Subject: [edk2-rfc] UEFI terminal console keyboard type extend for Putty

Hi everyone,

Putty is a popular terminal console software in windows and it support various types of terminal keyboard type. I would like to add most of the type support. Here is the key map info. Hope to get comments. Here is the type and mapping table:
Putty function key map:
+=========+======+===========+=============+=============+=============+
| | EFI | | | | |
| | Scan | | | Normal | |
| KEY | Code | VT100+ | Xterm R6 | VT400 | Linux |
+=========+======+===========+=============+=============+=============+
| F1 | 0x0B | ESC O P | NONE | ESC [ 1 1 ~ | ESC [ [ A |
| F2 | 0x0C | ESC O Q | NONE | ESC [ 1 2 ~ | ESC [ [ B |
| F3 | 0x0D | ESC O R | ESC O R | ESC [ 1 3 ~ | ESC [ [ C |
| F4 | 0x0E | ESC O S | ESC O S | ESC [ 1 3 ~ | ESC [ [ D |
| F5 | 0x0F | ESC O T | ESC [ 1 5 ~ | ESC [ 1 5 ~ | ESC [ [ E |
| F6 | 0x10 | ESC O U | ESC [ 1 7 ~ | ESC [ 1 7 ~ | ESC [ 1 7 ~ |
| F7 | 0x11 | ESC [ V | ESC [ 1 8 ~ | ESC [ 1 8 ~ | ESC [ 1 8 ~ |
| F8 | 0x12 | ESC [ W | ESC [ 1 9 ~ | ESC [ 1 9 ~ | ESC [ 1 9 ~ |
| F9 | 0x13 | ESC [ X | ESC [ 2 0 ~ | ESC [ 2 0 ~ | ESC [ 2 0 ~ |
| F10 | 0x14 | ESC [ Y | ESC [ 2 1 ~ | ESC [ 2 1 ~ | ESC [ 2 1 ~ |
| Escape | 0x17 | ESC | ESC | ESC | ESC |
| F11 | 0x15 | ESC O Z | ESC [ 2 3 ~ | ESC [ 2 3 ~ | ESC [ 1 3 ~ |
| F12 | 0x16 | ESC O [ | ESC [ 2 4 ~ | ESC [ 2 4 ~ | ESC [ 1 4 ~ |
+=========+======+===========+=============+=============+=============+

For Xterm R6 F1 and F2, it is descripted as ESC+O+P and ESC+O+Q, but it doesn't work on UEFI shell. Same with the SCO mode. So I would ignore the unworkable key map.
Info refer to https://www.ssh.com/ssh/putty/putty-manuals/0.68/Chapter4.html#config-funkeys

Thanks,
Zhichao


UEFI terminal console keyboard type extend for Putty

Gao, Zhichao
 

Hi everyone,

Putty is a popular terminal console software in windows and it support various types of terminal keyboard type. I would like to add most of the type support. Here is the key map info. Hope to get comments. Here is the type and mapping table:
Putty function key map:
+=========+======+===========+=============+=============+=============+
| | EFI | | | | |
| | Scan | | | Normal | |
| KEY | Code | VT100+ | Xterm R6 | VT400 | Linux |
+=========+======+===========+=============+=============+=============+
| F1 | 0x0B | ESC O P | NONE | ESC [ 1 1 ~ | ESC [ [ A |
| F2 | 0x0C | ESC O Q | NONE | ESC [ 1 2 ~ | ESC [ [ B |
| F3 | 0x0D | ESC O R | ESC O R | ESC [ 1 3 ~ | ESC [ [ C |
| F4 | 0x0E | ESC O S | ESC O S | ESC [ 1 3 ~ | ESC [ [ D |
| F5 | 0x0F | ESC O T | ESC [ 1 5 ~ | ESC [ 1 5 ~ | ESC [ [ E |
| F6 | 0x10 | ESC O U | ESC [ 1 7 ~ | ESC [ 1 7 ~ | ESC [ 1 7 ~ |
| F7 | 0x11 | ESC [ V | ESC [ 1 8 ~ | ESC [ 1 8 ~ | ESC [ 1 8 ~ |
| F8 | 0x12 | ESC [ W | ESC [ 1 9 ~ | ESC [ 1 9 ~ | ESC [ 1 9 ~ |
| F9 | 0x13 | ESC [ X | ESC [ 2 0 ~ | ESC [ 2 0 ~ | ESC [ 2 0 ~ |
| F10 | 0x14 | ESC [ Y | ESC [ 2 1 ~ | ESC [ 2 1 ~ | ESC [ 2 1 ~ |
| Escape | 0x17 | ESC | ESC | ESC | ESC |
| F11 | 0x15 | ESC O Z | ESC [ 2 3 ~ | ESC [ 2 3 ~ | ESC [ 1 3 ~ |
| F12 | 0x16 | ESC O [ | ESC [ 2 4 ~ | ESC [ 2 4 ~ | ESC [ 1 4 ~ |
+=========+======+===========+=============+=============+=============+

For Xterm R6 F1 and F2, it is descripted as ESC+O+P and ESC+O+Q, but it doesn't work on UEFI shell. Same with the SCO mode. So I would ignore the unworkable key map.
Info refer to https://www.ssh.com/ssh/putty/putty-manuals/0.68/Chapter4.html#config-funkeys

Thanks,
Zhichao


[PATCH] q35: lpc: allow to lock down 128K RAM at default SMBASE address

Igor Mammedov <imammedo@...>
 

lpc already has SMI negotiation feature, extend it by adding
optin ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT to supported features.

Writing this bit into "etc/smi/requested-features" fw_cfg file,
tells QEMU to alias 0x30000,128K RAM range into SMRAM address
space and mask this region from normal RAM address space
(reads return 0xff and writes are ignored, i.e. guest code
should be able to deal with not usable 0x30000,128K RAM range
once ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT is activated).

To make negotiated change effective, guest should read
"etc/smi/features-ok" fw_cfg file, which activates negotiated
features and locks down negotiating capabilities until hard reset.

Flow for initializing SMI handler on guest side:
1. set SMI handler entry point at default SMBASE location
2. check that host supports ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT
in "etc/smi/supported-features" and set if supported set
it in "etc/smi/requested-features"
3. read "etc/smi/features-ok", if returned value is 1
negotiated at step 2 features are activated successfully.

Signed-off-by: Igor Mammedov <imammedo@...>
---
include/hw/i386/ich9.h | 11 ++++++--
hw/i386/pc.c | 4 ++-
hw/i386/pc_q35.c | 3 ++-
hw/isa/lpc_ich9.c | 58 +++++++++++++++++++++++++++++++++++++++++-
4 files changed, 71 insertions(+), 5 deletions(-)

diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
index 72e803f6e2..c28685b753 100644
--- a/include/hw/i386/ich9.h
+++ b/include/hw/i386/ich9.h
@@ -12,11 +12,14 @@
#include "hw/acpi/acpi.h"
#include "hw/acpi/ich9.h"
#include "hw/pci/pci_bus.h"
+#include "qemu/units.h"

void ich9_lpc_set_irq(void *opaque, int irq_num, int level);
int ich9_lpc_map_irq(PCIDevice *pci_dev, int intx);
PCIINTxRoute ich9_route_intx_pin_to_irq(void *opaque, int pirq_pin);
-void ich9_lpc_pm_init(PCIDevice *pci_lpc, bool smm_enabled);
+void ich9_lpc_pm_init(PCIDevice *pci_lpc, bool smm_enabled,
+ MemoryRegion *system_memory, MemoryRegion *ram,
+ MemoryRegion *smram);
I2CBus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base);

void ich9_generate_smi(void);
@@ -71,6 +74,8 @@ typedef struct ICH9LPCState {
uint8_t smi_features_ok; /* guest-visible, read-only; selecting it
* triggers feature lockdown */
uint64_t smi_negotiated_features; /* guest-invisible, host endian */
+ MemoryRegion smbase_blackhole;
+ MemoryRegion smbase_window;

/* isa bus */
ISABus *isa_bus;
@@ -248,5 +253,7 @@ typedef struct ICH9LPCState {

/* bit positions used in fw_cfg SMI feature negotiation */
#define ICH9_LPC_SMI_F_BROADCAST_BIT 0
-
+#define ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT 1
+#define ICH9_LPC_SMBASE_ADDR 0x30000
+#define ICH9_LPC_SMBASE_RAM_SIZE (128 * KiB)
#endif /* HW_ICH9_H */
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c14ed86439..99a98303eb 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -119,7 +119,9 @@ struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
/* Physical Address of PVH entry point read from kernel ELF NOTE */
static size_t pvh_start_addr;

-GlobalProperty pc_compat_4_1[] = {};
+GlobalProperty pc_compat_4_1[] = {
+ { "ICH9-LPC", "x-smi-locked-smbase", "off" },
+};
const size_t pc_compat_4_1_len = G_N_ELEMENTS(pc_compat_4_1);

GlobalProperty pc_compat_4_0[] = {};
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index d4e8a1cb9f..50462686a0 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -292,7 +292,8 @@ static void pc_q35_init(MachineState *machine)
0xff0104);

/* connect pm stuff to lpc */
- ich9_lpc_pm_init(lpc, pc_machine_is_smm_enabled(pcms));
+ ich9_lpc_pm_init(lpc, pc_machine_is_smm_enabled(pcms), get_system_memory(),
+ ram_memory, MEMORY_REGION(object_resolve_path("/machine/smram", NULL)));

if (pcms->sata_enabled) {
/* ahci and SATA device, for q35 1 ahci controller is built-in */
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 17c292e306..17a8cd1b51 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -359,6 +359,38 @@ static void ich9_set_sci(void *opaque, int irq_num, int level)
}
}

+static uint64_t smbase_blackhole_read(void *ptr, hwaddr reg, unsigned size)
+{
+ return 0xffffffff;
+}
+
+static void smbase_blackhole_write(void *opaque, hwaddr addr, uint64_t val,
+ unsigned width)
+{
+ /* nothing */
+}
+
+static const MemoryRegionOps smbase_blackhole_ops = {
+ .read = smbase_blackhole_read,
+ .write = smbase_blackhole_write,
+ .endianness = DEVICE_NATIVE_ENDIAN,
+ .valid.min_access_size = 1,
+ .valid.max_access_size = 4,
+ .impl.min_access_size = 4,
+ .impl.max_access_size = 4,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void ich9_lpc_smbase_locked_update(ICH9LPCState *lpc)
+{
+ bool en = lpc->smi_negotiated_features & ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT;
+
+ memory_region_transaction_begin();
+ memory_region_set_enabled(&lpc->smbase_blackhole, en);
+ memory_region_set_enabled(&lpc->smbase_window, en);
+ memory_region_transaction_commit();
+}
+
static void smi_features_ok_callback(void *opaque)
{
ICH9LPCState *lpc = opaque;
@@ -379,9 +411,13 @@ static void smi_features_ok_callback(void *opaque)
/* valid feature subset requested, lock it down, report success */
lpc->smi_negotiated_features = guest_features;
lpc->smi_features_ok = 1;
+
+ ich9_lpc_smbase_locked_update(lpc);
}

-void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool smm_enabled)
+void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool smm_enabled,
+ MemoryRegion *system_memory, MemoryRegion *ram,
+ MemoryRegion *smram)
{
ICH9LPCState *lpc = ICH9_LPC_DEVICE(lpc_pci);
qemu_irq sci_irq;
@@ -413,6 +449,20 @@ void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool smm_enabled)
&lpc->smi_features_ok,
sizeof lpc->smi_features_ok,
true);
+
+ memory_region_init_io(&lpc->smbase_blackhole, OBJECT(lpc),
+ &smbase_blackhole_ops, NULL,
+ "smbase-blackhole", ICH9_LPC_SMBASE_RAM_SIZE);
+ memory_region_set_enabled(&lpc->smbase_blackhole, false);
+ memory_region_add_subregion_overlap(system_memory, ICH9_LPC_SMBASE_ADDR,
+ &lpc->smbase_blackhole, 1);
+
+
+ memory_region_init_alias(&lpc->smbase_window, OBJECT(lpc),
+ "smbase-window", ram,
+ ICH9_LPC_SMBASE_ADDR, ICH9_LPC_SMBASE_RAM_SIZE);
+ memory_region_set_enabled(&lpc->smbase_window, false);
+ memory_region_add_subregion(smram, 0x30000, &lpc->smbase_window);
}

ich9_lpc_reset(DEVICE(lpc));
@@ -508,6 +558,7 @@ static int ich9_lpc_post_load(void *opaque, int version_id)
ich9_lpc_pmbase_sci_update(lpc);
ich9_lpc_rcba_update(lpc, 0 /* disabled ICH9_LPC_RCBA_EN */);
ich9_lpc_pmcon_update(lpc);
+ ich9_lpc_smbase_locked_update(lpc);
return 0;
}

@@ -567,6 +618,8 @@ static void ich9_lpc_reset(DeviceState *qdev)
memset(lpc->smi_guest_features_le, 0, sizeof lpc->smi_guest_features_le);
lpc->smi_features_ok = 0;
lpc->smi_negotiated_features = 0;
+
+ ich9_lpc_smbase_locked_update(lpc);
}

/* root complex register block is mapped into memory space */
@@ -697,6 +750,7 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp)
qdev_init_gpio_out_named(dev, lpc->gsi, ICH9_GPIO_GSI, GSI_NUM_PINS);

isa_bus_irqs(isa_bus, lpc->gsi);
+
}

static bool ich9_rst_cnt_needed(void *opaque)
@@ -764,6 +818,8 @@ static Property ich9_lpc_properties[] = {
DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, true),
DEFINE_PROP_BIT64("x-smi-broadcast", ICH9LPCState, smi_host_features,
ICH9_LPC_SMI_F_BROADCAST_BIT, true),
+ DEFINE_PROP_BIT64("x-smi-locked-smbase", ICH9LPCState, smi_host_features,
+ ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT, true),
DEFINE_PROP_END_OF_LIST(),
};

--
2.18.1


Re: [Qemu-devel] [edk2-rfc] [edk2-devel] CPU hotplug using SMM with QEMU+OVMF

Igor Mammedov <imammedo@...>
 

On Thu, 5 Sep 2019 15:08:31 +0200
Laszlo Ersek <lersek@...> wrote:

On 09/04/19 11:52, Igor Mammedov wrote:

it could be stolen RAM + black hole like TSEG, assuming fw can live without RAM(0x30000+128K) range
(in this case fwcfg interface would only work for locking down the range)

or

we can actually have a dedicated SMRAM (like in my earlier RFC),
in this case FW can use RAM(0x30000+128K) when SMRAM isn't mapped into RAM address space
(in this case fwcfg would be used to temporarily map SMRAM into normal RAM and unmap/lock
after SMI relocation handler was initialized).

If possible I'd prefer a simpler TSEG like variant.
I think TSEG-like behavior is between these two. That is, I believe we
should have explicit open/close/lock operations. And, when the range is
closed (meaning, closed+unlocked, or closed+locked), then the black hole
should take effect for code that's not running in SMM.

Put differently, its like the second choice, except the range never
appears as normal RAM. "When SMRAM isn't mapped into RAM address space",
then the address range shows "nothing" (black hole).
I guess we at point where patch is better then words, I'll send one as reply here shortly.
I've just implemented subset of above (opened, closed+locked).


Regarding "fw can live without RAM(0x30000+128K) range" -- do you mean
whether the firmware could use another RAM area for fw_cfg DMA?

If that's the question, then I wouldn't worry about it. I'd remove the
0x30000+128K range from the memory map, so the fw_cfg stuff (or anything
else) would never allocate memory from the range. It's much more
concerning to me however how the SMM infrastructure would deal with a
hole in the memory map right there.
I didn't mean fwcfg in this context, what I meant if firmware were able
to avoid using RAM(0x30000+128K) range (since it becomes unusable after locking).
Looks like you just answered it here


Re: [Qemu-devel] [edk2-rfc] [edk2-devel] CPU hotplug using SMM with QEMU+OVMF

Laszlo Ersek
 

On 09/04/19 11:52, Igor Mammedov wrote:

it could be stolen RAM + black hole like TSEG, assuming fw can live without RAM(0x30000+128K) range
(in this case fwcfg interface would only work for locking down the range)

or

we can actually have a dedicated SMRAM (like in my earlier RFC),
in this case FW can use RAM(0x30000+128K) when SMRAM isn't mapped into RAM address space
(in this case fwcfg would be used to temporarily map SMRAM into normal RAM and unmap/lock
after SMI relocation handler was initialized).

If possible I'd prefer a simpler TSEG like variant.
I think TSEG-like behavior is between these two. That is, I believe we
should have explicit open/close/lock operations. And, when the range is
closed (meaning, closed+unlocked, or closed+locked), then the black hole
should take effect for code that's not running in SMM.

Put differently, its like the second choice, except the range never
appears as normal RAM. "When SMRAM isn't mapped into RAM address space",
then the address range shows "nothing" (black hole).


Regarding "fw can live without RAM(0x30000+128K) range" -- do you mean
whether the firmware could use another RAM area for fw_cfg DMA?

If that's the question, then I wouldn't worry about it. I'd remove the
0x30000+128K range from the memory map, so the fw_cfg stuff (or anything
else) would never allocate memory from the range. It's much more
concerning to me however how the SMM infrastructure would deal with a
hole in the memory map right there.

Thanks
Laszlo


Re: [edk2-devel] [RFC] EDK II Continuous Integration Phase 1

rebecca@...
 

On 2019-09-03 10:41, Ni, Ray wrote:
Can we change the process a bit?
1. maintainers created pull requests on behave of the patch owners
2. patch owners can be notified automatically if pull requests fail
3. patch owners update the pull requests
(I am not familiar to pull requests. I assume the rights of modifying pull requests and creating pull requests are separated. Are they?)

So, maintainers only need to initiate the pull requests. It assumes when pull requests are initiated, everyone at least agrees the justifications of the changes are valid and the ways the changes are done.
In other projects I've worked on, patch owners initiate the pre-commit
build/test, and either comment on the review (ideally with link to the
results) about its status, or have the CI system comment automatically
on the review page/thread.

Once the maintainer has signed off on the patch, it can then be sent for
gatekeeping, which is either a manual process whereby the repo owner (or
subsystem maintainer) checks the review, runs any additional tests they
want etc. - or via a 'land' command (for example with Review Board it's
"rbt land") which automates the checks and pushes the changeset.


I understand that with Azure Pipelines and how it integrates with
Github, along with the fact that this project doesn't use pull requests
or any other review automation, means things are probably going to be
pretty different to most other projects for now.


--

Rebecca Cran


Re: [Qemu-devel] [edk2-rfc] [edk2-devel] CPU hotplug using SMM with QEMU+OVMF

Igor Mammedov <imammedo@...>
 

On Tue, 3 Sep 2019 19:20:25 +0200
Laszlo Ersek <lersek@...> wrote:

On 09/03/19 16:53, Igor Mammedov wrote:
On Mon, 2 Sep 2019 21:09:58 +0200
Laszlo Ersek <lersek@...> wrote:

On 09/02/19 10:45, Igor Mammedov wrote:
On Fri, 30 Aug 2019 20:46:14 +0200
Laszlo Ersek <lersek@...> wrote:

On 08/30/19 16:48, Igor Mammedov wrote:

(01) On boot firmware maps and initializes SMI handler at default SMBASE (30000)
(using dedicated SMRAM at 30000 would allow us to avoid save/restore
steps and make SMM handler pointer not vulnerable to DMA attacks)

(02) QEMU hotplugs a new CPU in reset-ed state and sends SCI

(03) on receiving SCI, host CPU calls GPE cpu hotplug handler
which writes to IO port 0xB2 (broadcast SMI)

(04) firmware waits for all existing CPUs rendezvous in SMM mode,
new CPU(s) have SMI pending but does nothing yet

(05) host CPU wakes up one new CPU (INIT-INIT-SIPI)
SIPI vector points to RO flash HLT loop.
(how host CPU will know which new CPUs to relocate?
possibly reuse QEMU CPU hotplug MMIO interface???)

(06) new CPU does relocation.
(in case of attacker sends SIPI to several new CPUs,
open question how to detect collision of several CPUs at the same default SMBASE)

(07) once new CPU relocated host CPU completes initialization, returns
from IO port write and executes the rest of GPE handler, telling OS
to online new CPU.
In step (03), it is the OS that handles the SCI; it transfers control to
ACPI. The AML can write to IO port 0xB2 only because the OS allows it.

If the OS decides to omit that step, and sends an INIT-SIPI-SIPI
directly to the new CPU, can it steal the CPU?
It sure can but this way it won't get access to privileged SMRAM
so OS can't subvert firmware.
The next time SMI broadcast is sent the CPU will use SMI handler at
default 30000 SMBASE. It's up to us to define behavior here (for example
relocation handler can put such CPU in shutdown state).

It's in the best interest of OS to cooperate and execute AML
provided by firmware, if it does not follow proper cpu hotplug flow
we can't guarantee that stolen CPU will work.
This sounds convincing enough, for the hotplugged CPU; thanks.

So now my concern is with step (01). While preparing for the initial
relocation (of cold-plugged CPUs), the code assumes the memory at the
default SMBASE (0x30000) is normal RAM.

Is it not a problem that the area is written initially while running in
normal 32-bit or 64-bit mode, but then executed (in response to the
first, synchronous, SMI) as SMRAM?
currently there is no SMRAM at 0x30000, so all access falls through
into RAM address space and we are about to change that.

but firmware doesn't have to use it as RAM, it can check if QEMU
supports SMRAM at 0x30000 and if supported map it to configure
and then lock it down.
I'm sure you are *technically* right, but you seem to be assuming that I
can modify or rearrange anything I want in edk2. :)
yep, I'm looking at it from theoretical perspective so far,
but what could be done in reality might be limited.

If we can solve the above in OVMF platform code, that's great. If not
(e.g. UefiCpuPkg code needs to be updated), then things will get tricky.
If we can introduce another platform hook for this, that would help. I
can't say before I try.




Basically I'm confused by the alias.

TSEG (and presumably, A/B seg) work like this:
- when open, looks like RAM to normal mode and SMM
- when closed, looks like black-hole to normal mode, and like RAM to SMM

The generic edk2 code knows this, and manages the SMRAM areas accordingly.

The area at 0x30000 is different:
- looks like RAM to both normal mode and SMM

If we set up the alias at 0x30000 into A/B seg,
- will that *permanently* hide the normal RAM at 0x30000?
- will 0x30000 start behaving like A/B seg?

Basically my concern is that the universal code in edk2 might or might
not keep A/B seg open while initially populating the area at the default
SMBASE. Specifically, I can imagine two issues:

- if the alias into A/B seg is inactive during the initial population,
then the initial writes go to RAM, but the execution (the first SMBASE
relocation) will occur from A/B seg through the alias

- alternatively, if the alias is always active, but A/B seg is closed
during initial population (which happens in normal mode), then the
initial writes go to the black hole, and execution will occur from a
"blank" A/B seg.

Am I seeing things? (Sorry, I keep feeling dumber and dumber in this
thread.)
I don't really know how firmware uses A/B segments and I'm afraid that
cannibalizing one for configuring 0x30000 might break something.

Since we are inventing something out of q35 spec anyway, How about
leaving A/B/TSEG to be and using fwcfg to configure when/where
SMRAM(0x30000+128K) should be mapped into RAM address space.

I see a couple of options:
1: use identity mapping where SMRAM(0x30000+128K) maps into the same
range in RAM address space when firmware writes into fwcfg
file and unmaps/locks on the second write (until HW reset)
2: let firmware choose where to map SMRAM(0x30000+128K) in RAM address
space, logic is essentially the same as above only firmware
picks and writes into fwcfg an address where SMRAM(0x30000+128K)
should be mapped.
Option#1 would be similar to how TSEG works now, correct? IOW normal RAM
(from the QEMU perspective) is exposed as "SMRAM" to the guest, hidden
with a "black hole" overlay (outside of SMM) if SMRAM is closed.
it could be stolen RAM + black hole like TSEG, assuming fw can live without RAM(0x30000+128K) range
(in this case fwcfg interface would only work for locking down the range)

or

we can actually have a dedicated SMRAM (like in my earlier RFC),
in this case FW can use RAM(0x30000+128K) when SMRAM isn't mapped into RAM address space
(in this case fwcfg would be used to temporarily map SMRAM into normal RAM and unmap/lock
after SMI relocation handler was initialized).

If possible I'd prefer a simpler TSEG like variant.


If that's correct, then #1 looks more attractive to me than #2.

Thanks
Laszlo


Re: [Qemu-devel] [edk2-rfc] [edk2-devel] CPU hotplug using SMM with QEMU+OVMF

Igor Mammedov <imammedo@...>
 

On Mon, 2 Sep 2019 21:09:58 +0200
Laszlo Ersek <lersek@...> wrote:

On 09/02/19 10:45, Igor Mammedov wrote:
On Fri, 30 Aug 2019 20:46:14 +0200
Laszlo Ersek <lersek@...> wrote:

On 08/30/19 16:48, Igor Mammedov wrote:

(01) On boot firmware maps and initializes SMI handler at default SMBASE (30000)
(using dedicated SMRAM at 30000 would allow us to avoid save/restore
steps and make SMM handler pointer not vulnerable to DMA attacks)

(02) QEMU hotplugs a new CPU in reset-ed state and sends SCI

(03) on receiving SCI, host CPU calls GPE cpu hotplug handler
which writes to IO port 0xB2 (broadcast SMI)

(04) firmware waits for all existing CPUs rendezvous in SMM mode,
new CPU(s) have SMI pending but does nothing yet

(05) host CPU wakes up one new CPU (INIT-INIT-SIPI)
SIPI vector points to RO flash HLT loop.
(how host CPU will know which new CPUs to relocate?
possibly reuse QEMU CPU hotplug MMIO interface???)

(06) new CPU does relocation.
(in case of attacker sends SIPI to several new CPUs,
open question how to detect collision of several CPUs at the same default SMBASE)

(07) once new CPU relocated host CPU completes initialization, returns
from IO port write and executes the rest of GPE handler, telling OS
to online new CPU.
In step (03), it is the OS that handles the SCI; it transfers control to
ACPI. The AML can write to IO port 0xB2 only because the OS allows it.

If the OS decides to omit that step, and sends an INIT-SIPI-SIPI
directly to the new CPU, can it steal the CPU?
It sure can but this way it won't get access to privileged SMRAM
so OS can't subvert firmware.
The next time SMI broadcast is sent the CPU will use SMI handler at
default 30000 SMBASE. It's up to us to define behavior here (for example
relocation handler can put such CPU in shutdown state).

It's in the best interest of OS to cooperate and execute AML
provided by firmware, if it does not follow proper cpu hotplug flow
we can't guarantee that stolen CPU will work.
This sounds convincing enough, for the hotplugged CPU; thanks.

So now my concern is with step (01). While preparing for the initial
relocation (of cold-plugged CPUs), the code assumes the memory at the
default SMBASE (0x30000) is normal RAM.

Is it not a problem that the area is written initially while running in
normal 32-bit or 64-bit mode, but then executed (in response to the
first, synchronous, SMI) as SMRAM?
currently there is no SMRAM at 0x30000, so all access falls through
into RAM address space and we are about to change that.

but firmware doesn't have to use it as RAM, it can check if QEMU
supports SMRAM at 0x30000 and if supported map it to configure
and then lock it down.


Basically I'm confused by the alias.

TSEG (and presumably, A/B seg) work like this:
- when open, looks like RAM to normal mode and SMM
- when closed, looks like black-hole to normal mode, and like RAM to SMM

The generic edk2 code knows this, and manages the SMRAM areas accordingly.

The area at 0x30000 is different:
- looks like RAM to both normal mode and SMM

If we set up the alias at 0x30000 into A/B seg,
- will that *permanently* hide the normal RAM at 0x30000?
- will 0x30000 start behaving like A/B seg?

Basically my concern is that the universal code in edk2 might or might
not keep A/B seg open while initially populating the area at the default
SMBASE. Specifically, I can imagine two issues:

- if the alias into A/B seg is inactive during the initial population,
then the initial writes go to RAM, but the execution (the first SMBASE
relocation) will occur from A/B seg through the alias

- alternatively, if the alias is always active, but A/B seg is closed
during initial population (which happens in normal mode), then the
initial writes go to the black hole, and execution will occur from a
"blank" A/B seg.

Am I seeing things? (Sorry, I keep feeling dumber and dumber in this
thread.)
I don't really know how firmware uses A/B segments and I'm afraid that
cannibalizing one for configuring 0x30000 might break something.

Since we are inventing something out of q35 spec anyway, How about
leaving A/B/TSEG to be and using fwcfg to configure when/where
SMRAM(0x30000+128K) should be mapped into RAM address space.

I see a couple of options:
1: use identity mapping where SMRAM(0x30000+128K) maps into the same
range in RAM address space when firmware writes into fwcfg
file and unmaps/locks on the second write (until HW reset)
2: let firmware choose where to map SMRAM(0x30000+128K) in RAM address
space, logic is essentially the same as above only firmware
picks and writes into fwcfg an address where SMRAM(0x30000+128K)
should be mapped.


Anyway, I guess we could try and see if OVMF still boots with the alias...

Thanks
Laszlo


Re: [edk2-devel] [RFC] EDK II Continuous Integration Phase 1

Laszlo Ersek
 

On 09/03/19 19:09, Sean Brogan wrote:
Laszlo/Mike,

The idea that the maintainer must create the PR is fighting the
optimized github PR flow. Github and PRs process is optimized for
letting everyone contribute from "their" fork while still protecting
and putting process in place for the "upstream".

Why not use github to assign maintainers to each package or filepath
and then let contributors submit their own PR to edk2 after the
mailing list has approved. Then the PR has a policy that someone that
is the maintainer of all files changed must approve before the PR can
be completed (or you could even set it so that maintainer must
complete PR). This would have the benefit of less monotonous work
for the maintainers and on rejected PRs the contributor could easily
update their branch and resubmit their PR.
I'll let Mike respond.

Thanks
Laszlo


Re: [Qemu-devel] [edk2-rfc] [edk2-devel] CPU hotplug using SMM with QEMU+OVMF

Laszlo Ersek
 

On 09/03/19 16:53, Igor Mammedov wrote:
On Mon, 2 Sep 2019 21:09:58 +0200
Laszlo Ersek <lersek@...> wrote:

On 09/02/19 10:45, Igor Mammedov wrote:
On Fri, 30 Aug 2019 20:46:14 +0200
Laszlo Ersek <lersek@...> wrote:

On 08/30/19 16:48, Igor Mammedov wrote:

(01) On boot firmware maps and initializes SMI handler at default SMBASE (30000)
(using dedicated SMRAM at 30000 would allow us to avoid save/restore
steps and make SMM handler pointer not vulnerable to DMA attacks)

(02) QEMU hotplugs a new CPU in reset-ed state and sends SCI

(03) on receiving SCI, host CPU calls GPE cpu hotplug handler
which writes to IO port 0xB2 (broadcast SMI)

(04) firmware waits for all existing CPUs rendezvous in SMM mode,
new CPU(s) have SMI pending but does nothing yet

(05) host CPU wakes up one new CPU (INIT-INIT-SIPI)
SIPI vector points to RO flash HLT loop.
(how host CPU will know which new CPUs to relocate?
possibly reuse QEMU CPU hotplug MMIO interface???)

(06) new CPU does relocation.
(in case of attacker sends SIPI to several new CPUs,
open question how to detect collision of several CPUs at the same default SMBASE)

(07) once new CPU relocated host CPU completes initialization, returns
from IO port write and executes the rest of GPE handler, telling OS
to online new CPU.
In step (03), it is the OS that handles the SCI; it transfers control to
ACPI. The AML can write to IO port 0xB2 only because the OS allows it.

If the OS decides to omit that step, and sends an INIT-SIPI-SIPI
directly to the new CPU, can it steal the CPU?
It sure can but this way it won't get access to privileged SMRAM
so OS can't subvert firmware.
The next time SMI broadcast is sent the CPU will use SMI handler at
default 30000 SMBASE. It's up to us to define behavior here (for example
relocation handler can put such CPU in shutdown state).

It's in the best interest of OS to cooperate and execute AML
provided by firmware, if it does not follow proper cpu hotplug flow
we can't guarantee that stolen CPU will work.
This sounds convincing enough, for the hotplugged CPU; thanks.

So now my concern is with step (01). While preparing for the initial
relocation (of cold-plugged CPUs), the code assumes the memory at the
default SMBASE (0x30000) is normal RAM.

Is it not a problem that the area is written initially while running in
normal 32-bit or 64-bit mode, but then executed (in response to the
first, synchronous, SMI) as SMRAM?
currently there is no SMRAM at 0x30000, so all access falls through
into RAM address space and we are about to change that.

but firmware doesn't have to use it as RAM, it can check if QEMU
supports SMRAM at 0x30000 and if supported map it to configure
and then lock it down.
I'm sure you are *technically* right, but you seem to be assuming that I
can modify or rearrange anything I want in edk2. :)

If we can solve the above in OVMF platform code, that's great. If not
(e.g. UefiCpuPkg code needs to be updated), then things will get tricky.
If we can introduce another platform hook for this, that would help. I
can't say before I try.




Basically I'm confused by the alias.

TSEG (and presumably, A/B seg) work like this:
- when open, looks like RAM to normal mode and SMM
- when closed, looks like black-hole to normal mode, and like RAM to SMM

The generic edk2 code knows this, and manages the SMRAM areas accordingly.

The area at 0x30000 is different:
- looks like RAM to both normal mode and SMM

If we set up the alias at 0x30000 into A/B seg,
- will that *permanently* hide the normal RAM at 0x30000?
- will 0x30000 start behaving like A/B seg?

Basically my concern is that the universal code in edk2 might or might
not keep A/B seg open while initially populating the area at the default
SMBASE. Specifically, I can imagine two issues:

- if the alias into A/B seg is inactive during the initial population,
then the initial writes go to RAM, but the execution (the first SMBASE
relocation) will occur from A/B seg through the alias

- alternatively, if the alias is always active, but A/B seg is closed
during initial population (which happens in normal mode), then the
initial writes go to the black hole, and execution will occur from a
"blank" A/B seg.

Am I seeing things? (Sorry, I keep feeling dumber and dumber in this
thread.)
I don't really know how firmware uses A/B segments and I'm afraid that
cannibalizing one for configuring 0x30000 might break something.

Since we are inventing something out of q35 spec anyway, How about
leaving A/B/TSEG to be and using fwcfg to configure when/where
SMRAM(0x30000+128K) should be mapped into RAM address space.

I see a couple of options:
1: use identity mapping where SMRAM(0x30000+128K) maps into the same
range in RAM address space when firmware writes into fwcfg
file and unmaps/locks on the second write (until HW reset)
2: let firmware choose where to map SMRAM(0x30000+128K) in RAM address
space, logic is essentially the same as above only firmware
picks and writes into fwcfg an address where SMRAM(0x30000+128K)
should be mapped.
Option#1 would be similar to how TSEG works now, correct? IOW normal RAM
(from the QEMU perspective) is exposed as "SMRAM" to the guest, hidden
with a "black hole" overlay (outside of SMM) if SMRAM is closed.

If that's correct, then #1 looks more attractive to me than #2.

Thanks
Laszlo


Re: [edk2-devel] [RFC] EDK II Continuous Integration Phase 1

Sean
 

Laszlo/Mike,

The idea that the maintainer must create the PR is fighting the optimized github PR flow. Github and PRs process is optimized for letting everyone contribute from "their" fork while still protecting and putting process in place for the "upstream".

Why not use github to assign maintainers to each package or filepath and then let contributors submit their own PR to edk2 after the mailing list has approved. Then the PR has a policy that someone that is the maintainer of all files changed must approve before the PR can be completed (or you could even set it so that maintainer must complete PR). This would have the benefit of less monotonous work for the maintainers and on rejected PRs the contributor could easily update their branch and resubmit their PR.

Thanks
Sean

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Laszlo Ersek via Groups.Io
Sent: Tuesday, September 3, 2019 9:55 AM
To: Ni, Ray <ray.ni@...>; devel@edk2.groups.io; rfc@edk2.groups.io; Gao, Liming <liming.gao@...>; Kinney, Michael D <michael.d.kinney@...>
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous Integration Phase 1

On 09/03/19 18:41, Ni, Ray wrote:
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: Tuesday, September 3, 2019 6:20 AM
To: Ni, Ray <ray.ni@...>; rfc@edk2.groups.io;
devel@edk2.groups.io; Gao, Liming <liming.gao@...>; Kinney,
Michael D <michael.d.kinney@...>
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous
Integration Phase 1

On 09/03/19 05:39, Ni, Ray wrote:
Can someone draw a flow chart of who does what to help clarify?
I don't see this as a huge change, relative to the current process.

Before, it's always been up to the subsys maintainer to apply &
rebase the patches locally, pick up the feedback tags, and run at
least *some* build tests before pushing.

After, the final git push is not to "origin" but to a personal branch
on github.com, and then a github PR is needed. If that fails, notify
the submitter. That's all, as far as I can see.

It sounds like maintainers are going to be the very important bridges between CI system and the patch owners (developers).
Important it is I agree but boring it is as well I have to say.

Oh, it *is* absolutely boring.

Are maintainers still needed to be picked up/chosen/promoted from developers?
Would you trust a person to apply, build-test, and push your
UefiCpuPkg patches, if that person had *zero* UefiCpuPkg development experience?
(Or even zero edk2 development experience?)
Can we change the process a bit?
1. maintainers created pull requests on behave of the patch owners 2.
patch owners can be notified automatically if pull requests fail
I believe this can work if:
- the submitter has a github account
- the maintainer knows the submitter's account name
- the maintainer manually subscribes the submitter to the PR
(I have never tried this myself)

3. patch owners update the pull requests
(I am not familiar to pull requests. I assume the rights of
modifying pull requests and creating pull requests are separated. Are
they?)
PRs should not be updated. If there is a CI failure, the PR should be rejected. If that happens automatically, that's great. If not, then someone must close the PR manually. If the patch series submitter can do that, that's great (I have no idea if that works!) If only the PR owner (= maintainer) can close the PR, then they should close it.

Either way, the patch author will have to submit a v2 to the list. When that is sufficiently reviewed, the same process starts (new PR opened by maintainer).

So, maintainers only need to initiate the pull requests.
I hope this can actually work. We need to test this out first.

It assumes when pull requests are initiated, everyone at least agrees the justifications of the changes are valid and the ways the changes are done.
I agree fully about this. If the CI check completes, then the changes are pushed to master automatically! (Only if the push is a fast-forward
-- otherwise the PR is rejected again. GitHub will not be allowed to merge or rebase withoout supervision.)

Thanks
Laszlo


Re: [edk2-devel] [RFC] EDK II Continuous Integration Phase 1

Laszlo Ersek
 

On 09/03/19 18:41, Ni, Ray wrote:
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
Sent: Tuesday, September 3, 2019 6:20 AM
To: Ni, Ray <ray.ni@...>; rfc@edk2.groups.io; devel@edk2.groups.io; Gao, Liming <liming.gao@...>; Kinney,
Michael D <michael.d.kinney@...>
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous Integration Phase 1

On 09/03/19 05:39, Ni, Ray wrote:
Can someone draw a flow chart of who does what to help clarify?
I don't see this as a huge change, relative to the current process.

Before, it's always been up to the subsys maintainer to apply & rebase
the patches locally, pick up the feedback tags, and run at least *some*
build tests before pushing.

After, the final git push is not to "origin" but to a personal branch on
github.com, and then a github PR is needed. If that fails, notify the
submitter. That's all, as far as I can see.

It sounds like maintainers are going to be the very important bridges between CI system and the patch owners (developers).
Important it is I agree but boring it is as well I have to say.

Oh, it *is* absolutely boring.

Are maintainers still needed to be picked up/chosen/promoted from developers?
Would you trust a person to apply, build-test, and push your UefiCpuPkg
patches, if that person had *zero* UefiCpuPkg development experience?
(Or even zero edk2 development experience?)
Can we change the process a bit?
1. maintainers created pull requests on behave of the patch owners
2. patch owners can be notified automatically if pull requests fail
I believe this can work if:
- the submitter has a github account
- the maintainer knows the submitter's account name
- the maintainer manually subscribes the submitter to the PR
(I have never tried this myself)

3. patch owners update the pull requests
(I am not familiar to pull requests. I assume the rights of modifying pull requests and creating pull requests are separated. Are they?)
PRs should not be updated. If there is a CI failure, the PR should be
rejected. If that happens automatically, that's great. If not, then
someone must close the PR manually. If the patch series submitter can do
that, that's great (I have no idea if that works!) If only the PR owner
(= maintainer) can close the PR, then they should close it.

Either way, the patch author will have to submit a v2 to the list. When
that is sufficiently reviewed, the same process starts (new PR opened by
maintainer).

So, maintainers only need to initiate the pull requests.
I hope this can actually work. We need to test this out first.

It assumes when pull requests are initiated, everyone at least agrees the justifications of the changes are valid and the ways the changes are done.
I agree fully about this. If the CI check completes, then the changes
are pushed to master automatically! (Only if the push is a fast-forward
-- otherwise the PR is rejected again. GitHub will not be allowed to
merge or rebase withoout supervision.)

Thanks
Laszlo


Re: [edk2-devel] [RFC] EDK II Continuous Integration Phase 1

Ni, Ray
 

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
Sent: Tuesday, September 3, 2019 6:20 AM
To: Ni, Ray <ray.ni@...>; rfc@edk2.groups.io; devel@edk2.groups.io; Gao, Liming <liming.gao@...>; Kinney,
Michael D <michael.d.kinney@...>
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous Integration Phase 1

On 09/03/19 05:39, Ni, Ray wrote:
Can someone draw a flow chart of who does what to help clarify?
I don't see this as a huge change, relative to the current process.

Before, it's always been up to the subsys maintainer to apply & rebase
the patches locally, pick up the feedback tags, and run at least *some*
build tests before pushing.

After, the final git push is not to "origin" but to a personal branch on
github.com, and then a github PR is needed. If that fails, notify the
submitter. That's all, as far as I can see.

It sounds like maintainers are going to be the very important bridges between CI system and the patch owners (developers).
Important it is I agree but boring it is as well I have to say.

Oh, it *is* absolutely boring.

Are maintainers still needed to be picked up/chosen/promoted from developers?
Would you trust a person to apply, build-test, and push your UefiCpuPkg
patches, if that person had *zero* UefiCpuPkg development experience?
(Or even zero edk2 development experience?)
Can we change the process a bit?
1. maintainers created pull requests on behave of the patch owners
2. patch owners can be notified automatically if pull requests fail
3. patch owners update the pull requests
(I am not familiar to pull requests. I assume the rights of modifying pull requests and creating pull requests are separated. Are they?)

So, maintainers only need to initiate the pull requests. It assumes when pull requests are initiated, everyone at least agrees the justifications of the changes are valid and the ways the changes are done.

Thanks,
Ray


Re: [edk2-devel] [RFC] EDK II Continuous Integration Phase 1

Laszlo Ersek
 

On 09/03/19 05:39, Ni, Ray wrote:
Can someone draw a flow chart of who does what to help clarify?
I don't see this as a huge change, relative to the current process.

Before, it's always been up to the subsys maintainer to apply & rebase
the patches locally, pick up the feedback tags, and run at least *some*
build tests before pushing.

After, the final git push is not to "origin" but to a personal branch on
github.com, and then a github PR is needed. If that fails, notify the
submitter. That's all, as far as I can see.

It sounds like maintainers are going to be the very important bridges between CI system and the patch owners (developers). Important it is I agree but boring it is as well I have to say.
Oh, it *is* absolutely boring.

Are maintainers still needed to be picked up/chosen/promoted from developers?
Would you trust a person to apply, build-test, and push your UefiCpuPkg
patches, if that person had *zero* UefiCpuPkg development experience?
(Or even zero edk2 development experience?)

Maintainers don't have to be intimately familiar with the subsystem that
they are pushing a given set of patches for, but every bit of knowledge
helps.

So the answer is "technically no, but you'll regret it".

Maintainership is *always* a chore. It always takes away resources from
development activities, for the maintainer.

In most projects, people alternate in a given maintainer role -- they
keep replacing each other. The variance is mostly in how frequent this
alternation is.

In some projects, the role belongs to a group, and people cover the
maintainer responsibilities in very quick succession. In other projects,
people replace each other in maintainer roles when the current
maintainer gets tired of the work, and steps down. Then someone
volunteers to assume the role.

Either way, the new subsys maintainer is almost always a seasoned
developer in the subsystem.

Thanks,
Laszlo


-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Laszlo Ersek
Sent: Friday, August 30, 2019 5:58 AM
To: devel@edk2.groups.io; Gao, Liming <liming.gao@...>; rfc@edk2.groups.io; Kinney, Michael D
<michael.d.kinney@...>
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous Integration Phase 1

On 08/30/19 10:43, Liming Gao wrote:
Mike:
I add my comments.

-----Original Message-----
From: rfc@edk2.groups.io [mailto:rfc@edk2.groups.io] On Behalf Of Michael
D Kinney
Sent: Friday, August 30, 2019 4:23 AM
To: devel@edk2.groups.io; rfc@edk2.groups.io
Subject: [edk2-rfc] [RFC] EDK II Continuous Integration Phase 1

Hello,

This is a proposal for a first step towards continuous
integration for all TianoCore repositories to help
improve to quality of commits and automate testing and
release processes for all EDK II packages and platforms.

This is based on work from a number of EDK II community
members that have provide valuable input and evaluations.

* Rebecca Cran <rebecca@...> Jenkins evaluation
* Laszlo Ersek <lersek@...> GitLab evaluation
* Philippe Mathieu-Daudé <philmd@...> GitLab evaluation
* Sean Brogan <sean.brogan@...> Azure Pipelines and HBFA
* Bret Barkelew <Bret.Barkelew@...> Azure Pipelines and HBFA
* Jiewen Yao <jiewen.yao@...> HBFA

The following link is a link to an EDK II WIKI page that
contains a summary of the work to date. Please provide
feedback in the EDK II mailing lists. The WIKI pages will
be updated with input from the entire EDK II community.

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Continuous-
Integration

Proposal
========
Phase 1 of adding continuous integration is limited to the
edk2 repository. Additional repositories will be added later.

The following changes are proposed:
* Remove EDK II Maintainers write access to edk2 repository.
Only EDK II Administrators will continue to have write
access, and that should only be used to handle extraordinary
events.
* EDK II Maintainers use a GitHub Pull Request instead of push
to commit a patch series to the edk2 repository. There are
no other changes to the development and review process. The
patch series is prepared in an EDK II maintainer branch with
all commit message requirements met on each patch in the series.
Will the maintainer manually provide pull request after the patch passes the review?
Yes. The maintainer will prepare a local branch that is rebased to
master, and has all the mailing list feedback tags (Reviewed-by, etc)
applied. The maintainer also does all the local testing that they
usually do, just before the final "git push origin".

Then, the final "git push origin" action is replaced by:
(1) git push personal-repo topic-branch-pr
(2) manual creation of a GitHub.com Pull Request, for the topic branch
just pushed.

That is, the final "git push origin" action is replaced with the pushing
of a personal (ready-made) topic branch, and a GitHub.com Pull Request
against that branch. The verification and the final merging will be
performed by github.

Can the script scan the mail list and auto trig pull request once the patch gets
Reviewed-by or Acked-by from Package maintainers?
No, it can't. (And, at this stage, it should not.) The coordination
between submitters, maintainers, reviewers, and occasionally, stewards,
for determining when a patch series is ready to go, based on review
discussion, remains the same.

* The edk2 repository only accepts Pull Requests from members
of the EDK II Maintainers team. Pull Requests from anyone else
are rejected.
* Run pre-commit checks using Azure Pipelines
The maintainer manually trig pre-commit check or auto trig pre-commit?
After the maintainer pushes the ready-made branch to their personal
repo, and submits a PR against that, the PR will set off the checks. If
the checks pass, the topic branch is merged.

By default, pre-commit should be auto trigged based on pull request.

* If all pre-commit checks pass, then the patch series is auto
committed. The result of this commit must match the contents
and commit history that would have occurred using the previous
push operation.
* If any pre-commit checks fail, then notify the submitter.
Will Pre-commit check fail send the mail to the patch submitter?
The patch submitter need update the patch and go through review process again.
My understanding is that, if the testing in GitHub.com fails, the PR
will be rejected and closed. This will generate a notification email for
the maintainer that submitted the PR. The maintainer can then return to
the email thread, and report the CI failure, possibly with a link to the
failed / rejected PR.

Then, indeed, the submitter must rework the series and post a new
version to the list.

It's also possible (and polite) if the maintainer posts the PR link in
the mailing list thread right after submitting the PR. Then the
submitter can monitor the PR too. (Subscribe to it for notifications.)
As I understand it.

Furthermore,


A typical reason for a failure would be a merge conflict with
another pull request that was just processed.
* Limit pre-commit checks execution time to 10 minutes.
* Provide on-demand builds to EDK II Maintainers that to allow
EDK II Maintainers to submit a branch through for the same
set of pre-commit checks without submitting a pull request.
a maintainer could use this on-demand build & testing facility in the
course of review, to report findings early.

Thanks
Laszlo


## Pre-Commit Checks in Phase 1
* Run and pass PatchCheck.py with no errors

=====================================================

The following are some additional pre-commit check ideas
that could be quickly added once the initial version using
PatchCheck.py is fully functional. Please provide feedback
on the ones you like and additional ones you think may
improve the quality of the commits to the edk2 repository.

## Proposed Pre-Commit Checks in Phase 2
* Verify Reviewed-by and Acked-by tags are present with
correct maintainer email addresses
* Verify no non-ASCII characters in modified files
* Verify no binary files in set of modified files
Now, Edk2 has few binary files, like logo.bmp.
I see one BZ to request logo bmp update.
(BZ https://bugzilla.tianocore.org/show_bug.cgi?id=2050)
So, we need the exception way for it.

* Verify package dependency rules in modified files

## Proposed Pre-Commit Checks in Phase 3
* Run ECC on modified files
* Verify modified modules/libs build
* Run available host based tests (HBFA) against modified
modules/libs
I request boot test on Emulator and Ovmf in the daily and weekly scope.
Daily can cover boot to Shell.
Weekly can cover more boot functionality.


Re: [edk2-devel] [RFC] EDK II Continuous Integration Phase 1

Ni, Ray
 

Can someone draw a flow chart of who does what to help clarify?

It sounds like maintainers are going to be the very important bridges between CI system and the patch owners (developers). Important it is I agree but boring it is as well I have to say.

Are maintainers still needed to be picked up/chosen/promoted from developers?

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Laszlo Ersek
Sent: Friday, August 30, 2019 5:58 AM
To: devel@edk2.groups.io; Gao, Liming <liming.gao@...>; rfc@edk2.groups.io; Kinney, Michael D
<michael.d.kinney@...>
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous Integration Phase 1

On 08/30/19 10:43, Liming Gao wrote:
Mike:
I add my comments.

-----Original Message-----
From: rfc@edk2.groups.io [mailto:rfc@edk2.groups.io] On Behalf Of Michael
D Kinney
Sent: Friday, August 30, 2019 4:23 AM
To: devel@edk2.groups.io; rfc@edk2.groups.io
Subject: [edk2-rfc] [RFC] EDK II Continuous Integration Phase 1

Hello,

This is a proposal for a first step towards continuous
integration for all TianoCore repositories to help
improve to quality of commits and automate testing and
release processes for all EDK II packages and platforms.

This is based on work from a number of EDK II community
members that have provide valuable input and evaluations.

* Rebecca Cran <rebecca@...> Jenkins evaluation
* Laszlo Ersek <lersek@...> GitLab evaluation
* Philippe Mathieu-Daudé <philmd@...> GitLab evaluation
* Sean Brogan <sean.brogan@...> Azure Pipelines and HBFA
* Bret Barkelew <Bret.Barkelew@...> Azure Pipelines and HBFA
* Jiewen Yao <jiewen.yao@...> HBFA

The following link is a link to an EDK II WIKI page that
contains a summary of the work to date. Please provide
feedback in the EDK II mailing lists. The WIKI pages will
be updated with input from the entire EDK II community.

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Continuous-
Integration

Proposal
========
Phase 1 of adding continuous integration is limited to the
edk2 repository. Additional repositories will be added later.

The following changes are proposed:
* Remove EDK II Maintainers write access to edk2 repository.
Only EDK II Administrators will continue to have write
access, and that should only be used to handle extraordinary
events.
* EDK II Maintainers use a GitHub Pull Request instead of push
to commit a patch series to the edk2 repository. There are
no other changes to the development and review process. The
patch series is prepared in an EDK II maintainer branch with
all commit message requirements met on each patch in the series.
Will the maintainer manually provide pull request after the patch passes the review?
Yes. The maintainer will prepare a local branch that is rebased to
master, and has all the mailing list feedback tags (Reviewed-by, etc)
applied. The maintainer also does all the local testing that they
usually do, just before the final "git push origin".

Then, the final "git push origin" action is replaced by:
(1) git push personal-repo topic-branch-pr
(2) manual creation of a GitHub.com Pull Request, for the topic branch
just pushed.

That is, the final "git push origin" action is replaced with the pushing
of a personal (ready-made) topic branch, and a GitHub.com Pull Request
against that branch. The verification and the final merging will be
performed by github.

Can the script scan the mail list and auto trig pull request once the patch gets
Reviewed-by or Acked-by from Package maintainers?
No, it can't. (And, at this stage, it should not.) The coordination
between submitters, maintainers, reviewers, and occasionally, stewards,
for determining when a patch series is ready to go, based on review
discussion, remains the same.

* The edk2 repository only accepts Pull Requests from members
of the EDK II Maintainers team. Pull Requests from anyone else
are rejected.
* Run pre-commit checks using Azure Pipelines
The maintainer manually trig pre-commit check or auto trig pre-commit?
After the maintainer pushes the ready-made branch to their personal
repo, and submits a PR against that, the PR will set off the checks. If
the checks pass, the topic branch is merged.

By default, pre-commit should be auto trigged based on pull request.

* If all pre-commit checks pass, then the patch series is auto
committed. The result of this commit must match the contents
and commit history that would have occurred using the previous
push operation.
* If any pre-commit checks fail, then notify the submitter.
Will Pre-commit check fail send the mail to the patch submitter?
The patch submitter need update the patch and go through review process again.
My understanding is that, if the testing in GitHub.com fails, the PR
will be rejected and closed. This will generate a notification email for
the maintainer that submitted the PR. The maintainer can then return to
the email thread, and report the CI failure, possibly with a link to the
failed / rejected PR.

Then, indeed, the submitter must rework the series and post a new
version to the list.

It's also possible (and polite) if the maintainer posts the PR link in
the mailing list thread right after submitting the PR. Then the
submitter can monitor the PR too. (Subscribe to it for notifications.)
As I understand it.

Furthermore,


A typical reason for a failure would be a merge conflict with
another pull request that was just processed.
* Limit pre-commit checks execution time to 10 minutes.
* Provide on-demand builds to EDK II Maintainers that to allow
EDK II Maintainers to submit a branch through for the same
set of pre-commit checks without submitting a pull request.
a maintainer could use this on-demand build & testing facility in the
course of review, to report findings early.

Thanks
Laszlo


## Pre-Commit Checks in Phase 1
* Run and pass PatchCheck.py with no errors

=====================================================

The following are some additional pre-commit check ideas
that could be quickly added once the initial version using
PatchCheck.py is fully functional. Please provide feedback
on the ones you like and additional ones you think may
improve the quality of the commits to the edk2 repository.

## Proposed Pre-Commit Checks in Phase 2
* Verify Reviewed-by and Acked-by tags are present with
correct maintainer email addresses
* Verify no non-ASCII characters in modified files
* Verify no binary files in set of modified files
Now, Edk2 has few binary files, like logo.bmp.
I see one BZ to request logo bmp update.
(BZ https://bugzilla.tianocore.org/show_bug.cgi?id=2050)
So, we need the exception way for it.

* Verify package dependency rules in modified files

## Proposed Pre-Commit Checks in Phase 3
* Run ECC on modified files
* Verify modified modules/libs build
* Run available host based tests (HBFA) against modified
modules/libs
I request boot test on Emulator and Ovmf in the daily and weekly scope.
Daily can cover boot to Shell.
Weekly can cover more boot functionality.


Re: [edk2-devel] CPU hotplug using SMM with QEMU+OVMF

Laszlo Ersek
 

On 09/02/19 10:45, Igor Mammedov wrote:
On Fri, 30 Aug 2019 20:46:14 +0200
Laszlo Ersek <lersek@...> wrote:

On 08/30/19 16:48, Igor Mammedov wrote:

(01) On boot firmware maps and initializes SMI handler at default SMBASE (30000)
(using dedicated SMRAM at 30000 would allow us to avoid save/restore
steps and make SMM handler pointer not vulnerable to DMA attacks)

(02) QEMU hotplugs a new CPU in reset-ed state and sends SCI

(03) on receiving SCI, host CPU calls GPE cpu hotplug handler
which writes to IO port 0xB2 (broadcast SMI)

(04) firmware waits for all existing CPUs rendezvous in SMM mode,
new CPU(s) have SMI pending but does nothing yet

(05) host CPU wakes up one new CPU (INIT-INIT-SIPI)
SIPI vector points to RO flash HLT loop.
(how host CPU will know which new CPUs to relocate?
possibly reuse QEMU CPU hotplug MMIO interface???)

(06) new CPU does relocation.
(in case of attacker sends SIPI to several new CPUs,
open question how to detect collision of several CPUs at the same default SMBASE)

(07) once new CPU relocated host CPU completes initialization, returns
from IO port write and executes the rest of GPE handler, telling OS
to online new CPU.
In step (03), it is the OS that handles the SCI; it transfers control to
ACPI. The AML can write to IO port 0xB2 only because the OS allows it.

If the OS decides to omit that step, and sends an INIT-SIPI-SIPI
directly to the new CPU, can it steal the CPU?
It sure can but this way it won't get access to privileged SMRAM
so OS can't subvert firmware.
The next time SMI broadcast is sent the CPU will use SMI handler at
default 30000 SMBASE. It's up to us to define behavior here (for example
relocation handler can put such CPU in shutdown state).

It's in the best interest of OS to cooperate and execute AML
provided by firmware, if it does not follow proper cpu hotplug flow
we can't guarantee that stolen CPU will work.
This sounds convincing enough, for the hotplugged CPU; thanks.

So now my concern is with step (01). While preparing for the initial
relocation (of cold-plugged CPUs), the code assumes the memory at the
default SMBASE (0x30000) is normal RAM.

Is it not a problem that the area is written initially while running in
normal 32-bit or 64-bit mode, but then executed (in response to the
first, synchronous, SMI) as SMRAM?

Basically I'm confused by the alias.

TSEG (and presumably, A/B seg) work like this:
- when open, looks like RAM to normal mode and SMM
- when closed, looks like black-hole to normal mode, and like RAM to SMM

The generic edk2 code knows this, and manages the SMRAM areas accordingly.

The area at 0x30000 is different:
- looks like RAM to both normal mode and SMM

If we set up the alias at 0x30000 into A/B seg,
- will that *permanently* hide the normal RAM at 0x30000?
- will 0x30000 start behaving like A/B seg?

Basically my concern is that the universal code in edk2 might or might
not keep A/B seg open while initially populating the area at the default
SMBASE. Specifically, I can imagine two issues:

- if the alias into A/B seg is inactive during the initial population,
then the initial writes go to RAM, but the execution (the first SMBASE
relocation) will occur from A/B seg through the alias

- alternatively, if the alias is always active, but A/B seg is closed
during initial population (which happens in normal mode), then the
initial writes go to the black hole, and execution will occur from a
"blank" A/B seg.

Am I seeing things? (Sorry, I keep feeling dumber and dumber in this
thread.)

Anyway, I guess we could try and see if OVMF still boots with the alias...

Thanks
Laszlo


Re: [edk2-devel] CPU hotplug using SMM with QEMU+OVMF

Igor Mammedov <imammedo@...>
 

On Fri, 30 Aug 2019 20:46:14 +0200
Laszlo Ersek <lersek@...> wrote:

On 08/30/19 16:48, Igor Mammedov wrote:

(01) On boot firmware maps and initializes SMI handler at default SMBASE (30000)
(using dedicated SMRAM at 30000 would allow us to avoid save/restore
steps and make SMM handler pointer not vulnerable to DMA attacks)

(02) QEMU hotplugs a new CPU in reset-ed state and sends SCI

(03) on receiving SCI, host CPU calls GPE cpu hotplug handler
which writes to IO port 0xB2 (broadcast SMI)

(04) firmware waits for all existing CPUs rendezvous in SMM mode,
new CPU(s) have SMI pending but does nothing yet

(05) host CPU wakes up one new CPU (INIT-INIT-SIPI)
SIPI vector points to RO flash HLT loop.
(how host CPU will know which new CPUs to relocate?
possibly reuse QEMU CPU hotplug MMIO interface???)

(06) new CPU does relocation.
(in case of attacker sends SIPI to several new CPUs,
open question how to detect collision of several CPUs at the same default SMBASE)

(07) once new CPU relocated host CPU completes initialization, returns
from IO port write and executes the rest of GPE handler, telling OS
to online new CPU.
In step (03), it is the OS that handles the SCI; it transfers control to
ACPI. The AML can write to IO port 0xB2 only because the OS allows it.

If the OS decides to omit that step, and sends an INIT-SIPI-SIPI
directly to the new CPU, can it steal the CPU?
It sure can but this way it won't get access to privileged SMRAM
so OS can't subvert firmware.
The next time SMI broadcast is sent the CPU will use SMI handler at
default 30000 SMBASE. It's up to us to define behavior here (for example
relocation handler can put such CPU in shutdown state).

It's in the best interest of OS to cooperate and execute AML
provided by firmware, if it does not follow proper cpu hotplug flow
we can't guarantee that stolen CPU will work.

Thanks!
Laszlo


Re: [RFC] EDK II Continuous Integration Phase 1

rebecca@...
 

On 2019-08-29 14:22, Michael D Kinney wrote:
This is based on work from a number of EDK II community
members that have provide valuable input and evaluations.

* Rebecca Cran <rebecca@...> Jenkins evaluation
* Laszlo Ersek <lersek@...> GitLab evaluation
* Philippe Mathieu-Daudé <philmd@...> GitLab evaluation
* Sean Brogan <sean.brogan@...> Azure Pipelines and HBFA
* Bret Barkelew <Bret.Barkelew@...> Azure Pipelines and HBFA
* Jiewen Yao <jiewen.yao@...> HBFA

The following link is a link to an EDK II WIKI page that
contains a summary of the work to date. Please provide
feedback in the EDK II mailing lists. The WIKI pages will
be updated with input from the entire EDK II community.

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Continuous-Integration


Sorry, I was thinking the evaluation I was doing was having the entire
CI run in Jenkins, not only the boot testing. I don't have the array of
hardware you'd want for boot testing, just mostly large x86 systems. 

So I think I'll drop out of the evaluation.


--

Rebecca Cran


Re: [edk2-devel] CPU hotplug using SMM with QEMU+OVMF

Laszlo Ersek
 

On 08/30/19 16:48, Igor Mammedov wrote:

(01) On boot firmware maps and initializes SMI handler at default SMBASE (30000)
(using dedicated SMRAM at 30000 would allow us to avoid save/restore
steps and make SMM handler pointer not vulnerable to DMA attacks)

(02) QEMU hotplugs a new CPU in reset-ed state and sends SCI

(03) on receiving SCI, host CPU calls GPE cpu hotplug handler
which writes to IO port 0xB2 (broadcast SMI)

(04) firmware waits for all existing CPUs rendezvous in SMM mode,
new CPU(s) have SMI pending but does nothing yet

(05) host CPU wakes up one new CPU (INIT-INIT-SIPI)
SIPI vector points to RO flash HLT loop.
(how host CPU will know which new CPUs to relocate?
possibly reuse QEMU CPU hotplug MMIO interface???)

(06) new CPU does relocation.
(in case of attacker sends SIPI to several new CPUs,
open question how to detect collision of several CPUs at the same default SMBASE)

(07) once new CPU relocated host CPU completes initialization, returns
from IO port write and executes the rest of GPE handler, telling OS
to online new CPU.
In step (03), it is the OS that handles the SCI; it transfers control to
ACPI. The AML can write to IO port 0xB2 only because the OS allows it.

If the OS decides to omit that step, and sends an INIT-SIPI-SIPI
directly to the new CPU, can it steal the CPU?

Thanks!
Laszlo


Re: [edk2-devel] CPU hotplug using SMM with QEMU+OVMF

Igor Mammedov <imammedo@...>
 

On Thu, 29 Aug 2019 19:01:35 +0200
Laszlo Ersek <lersek@...> wrote:

On 08/27/19 20:31, Igor Mammedov wrote:
On Sat, 24 Aug 2019 01:48:09 +0000
"Yao, Jiewen" <jiewen.yao@...> wrote:
(05) Host CPU: (OS) Port 0xB2 write, all CPUs enter SMM (NOTE: New CPU
will not enter CPU because SMI is disabled)
I think only CPU that does the write will enter SMM
That used to be the case (and it is still the default QEMU behavior, if
broadcast SMI is not negotiated). However, OVMF does negotiate broadcast
SMI whenever QEMU offers the feature. Broadcast SMI is important for the
stability of the edk2 SMM infrastructure on QEMU/KVM, we've found.

https://bugzilla.redhat.com/show_bug.cgi?id=1412313
https://bugzilla.redhat.com/show_bug.cgi?id=1412327

and we might not need to pull in all already initialized CPUs into SMM.
That, on the other hand, could be a valid idea. But then the CPU should
use a different method for raising a synchronous SMI for itself (not a
write to IO port 0xB2). Is a "directed SMI for self" possible?
theoretically depending on argument in 0xb3, it should be possible to
rise directed SMI even if broadcast ones are negotiated.

[...]
I've tried to read through the procedure with your suggested changes,
but I'm failing at composing a coherent mental image, in this email
response format.

If you have the time, can you write up the suggested list of steps in a
"flat" format? (I believe you are suggesting to eliminate some steps
completely.)
if I'd sum it up:

(01) On boot firmware maps and initializes SMI handler at default SMBASE (30000)
(using dedicated SMRAM at 30000 would allow us to avoid save/restore
steps and make SMM handler pointer not vulnerable to DMA attacks)

(02) QEMU hotplugs a new CPU in reset-ed state and sends SCI

(03) on receiving SCI, host CPU calls GPE cpu hotplug handler
which writes to IO port 0xB2 (broadcast SMI)

(04) firmware waits for all existing CPUs rendezvous in SMM mode,
new CPU(s) have SMI pending but does nothing yet

(05) host CPU wakes up one new CPU (INIT-INIT-SIPI)
SIPI vector points to RO flash HLT loop.
(how host CPU will know which new CPUs to relocate?
possibly reuse QEMU CPU hotplug MMIO interface???)

(06) new CPU does relocation.
(in case of attacker sends SIPI to several new CPUs,
open question how to detect collision of several CPUs at the same default SMBASE)

(07) once new CPU relocated host CPU completes initialization, returns
from IO port write and executes the rest of GPE handler, telling OS
to online new CPU.


... jumping to another point:

2) Let trusted software (SMM and init code) guarantee SMREBASE one by one (include any code runs before SMREBASE)
that would mean pulling all present CPUs into SMM mode so no attack
code could be executing before doing hotplug. With a lot of present CPUs
it could be quite expensive and unlike physical hardware, guest's CPUs
could be preempted arbitrarily long causing long delays.
I agree with your analysis, but I slightly disagree about the impact:

- CPU hotplug is not a frequent administrative action, so the CPU load
should be temporary (it should be a spike). I don't worry that it would
trip up OS kernel code. (SMI handling is known to take long on physical
platforms oo.) In practice, all "normal" SMIs are broadcast already (for
example when calling the runtime UEFI variable services from the OS kernel).

- The fact that QEMU/KVM introduces some jitter into the execution of
multi-core code (including SMM code) has proved useful in the past, for
catching edk2 regressions.

Again, this is not a strong disagreement from my side. I'm open to
better ways for synching CPUs during muti-CPU-hotplug.

(Digression:

I expect someone could be curious why (a) I find it acceptable (even
beneficial) that "some jitter" injected by the QEMU/KVM scheduling
exposes multi-core regressions in edk2, but at the same time (b) I found
it really important to add broadcast SMI to QEMU and OVMF. After all,
both "jitter" and "unicast SMIs" are QEMU/KVM platform specifics, so why
the different treatment?

The reason is that the "jitter" does not interfere with normal
operation, and it has been good for catching *regressions*. IOW, there
is a working edk2 state, someone posts a patch, works on physical
hardware, but breaks on QEMU/KVM --> then we can still reject or rework
or revert the patch. And we're back to a working state again (in the
best case, with a fixed feature patch).

With the unicast SMIs however, it was impossible to enable the SMM stack
reliably in the first place. There was no functional state to return to.
I don't really get the last statement, but the I know nothing about OVMF.
I don't insist on unicast SMI being used, it's just some ideas about what
we could do. It could be done later, broadcast SMI (might be not the best)
is sufficient to implement CPU hotplug.

Digression ends.)

lets first see if if we can ignore race
Makes me uncomfortable, but if this is the consensus, I'll go along.
same here, as mentioned in another reply as it's only possible in
attack case (multiple SMIs + multiple SIPI) so it could be fine to just
explode in case it happens (point is fw in not leaking anything from SMRAM
and OS did something illegeal).

and if it's not then
we probably end up with implementing some form of #1
OK.

Thanks!
Laszlo

661 - 680 of 780