Re: [PATCH 3/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Andrew Jones]

On Tue, Jul 14, 2020 at 04:41:41PM +0200, Igor Mammedov wrote:
> On Tue, 14 Jul 2020 11:23:25 +0200
> Andrew Jones <drjones@redhat.com> wrote:
> 
> > On Tue, Jul 14, 2020 at 04:57:50AM -0400, Michael S. Tsirkin wrote:
> > > On Tue, Jul 14, 2020 at 07:51:09AM +0200, Andrew Jones wrote:  
> > > > On Mon, Jul 13, 2020 at 10:49:07AM +0200, Igor Mammedov wrote:  
> > > > > On Mon, 29 Jun 2020 16:09:37 +0200
> > > > > Andrew Jones <drjones@redhat.com> wrote:
> > > > >   
> > > > > > The flash device is exclusively for the host-controlled firmware, so
> > > > > > we should not expose it to the OS. Exposing it risks the OS messing
> > > > > > with it, which could break firmware runtime services and surprise 
> > > > > > the
> > > > > > OS when all its changes disappear after reboot.
> > > > > > 
> > > > > > As firmware needs the device and uses DT, we leave the device 
> > > > > > exposed
> > > > > > there. It's up to firmware to remove the nodes from DT before 
> > > > > > sending
> > > > > > it on to the OS. However, there's no need to force firmware to 
> > > > > > remove
> > > > > > tables from ACPI (which it doesn't know how to do anyway), so we
> > > > > > simply don't add the tables in the first place. But, as we've been
> > > > > > adding the tables for quite some time and don't want to change the
> > > > > > default hardware exposed to versioned machines, then we only stop
> > > > > > exposing the flash device tables for 5.1 and later machine types.
> > > > > > 
> > > > > > Suggested-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > > > > Suggested-by: Laszlo Ersek <lersek@redhat.com>
> > > > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > > > ---
> > > > > >  hw/arm/virt-acpi-build.c | 5 ++++-
> > > > > >  hw/arm/virt.c            | 3 +++
> > > > > >  include/hw/arm/virt.h    | 1 +
> > > > > >  3 files changed, 8 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > > > > index 1384a2cf2ab4..91f0df7b13a3 100644
> > > > > > --- a/hw/arm/virt-acpi-build.c
> > > > > > +++ b/hw/arm/virt-acpi-build.c
> > > > > > @@ -749,6 +749,7 @@ static void build_fadt_rev5(GArray *table_data, 
> > > > > > BIOSLinker *linker,
> > > > > >  static void
> > > > > >  build_dsdt(GArray *table_data, BIOSLinker *linker, 
> > > > > > VirtMachineState *vms)
> > > > > >  {
> > > > > > +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> > > > > >      Aml *scope, *dsdt;
> > > > > >      MachineState *ms = MACHINE(vms);
> > > > > >      const MemMapEntry *memmap = vms->memmap;
> > > > > > @@ -767,7 +768,9 @@ build_dsdt(GArray *table_data, BIOSLinker 
> > > > > > *linker, VirtMachineState *vms)
> > > > > >      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
> > > > > >      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
> > > > > >                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> > > > > > -    acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> > > > > > +    if (vmc->acpi_expose_flash) {
> > > > > > +        acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> > > > > > +    }
> > > > > >      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
> > > > > >      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
> > > > > >                      (irqmap[VIRT_MMIO] + ARM_SPI_BASE), 
> > > > > > NUM_VIRTIO_TRANSPORTS);
> > > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > > > > index cd0834ce7faf..5adc9ff799ef 100644
> > > > > > --- a/hw/arm/virt.c
> > > > > > +++ b/hw/arm/virt.c
> > > > > > @@ -2482,9 +2482,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 1)
> > > > > >  
> > > > > >  static void virt_machine_5_0_options(MachineClass *mc)
> > > > > >  {
> > > > > > +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> > > > > > +
> > > > > >      virt_machine_5_1_options(mc);
> > > > > >      compat_props_add(mc->compat_props, hw_compat_5_0, 
> > > > > > hw_compat_5_0_len);
> > > > > >      mc->numa_mem_supported = true;
> > > > > > +    vmc->acpi_expose_flash = true;  
> > > > > 
> > > > > we usually do not version ACPI tables changes
> > > > > (unless we have a good reason to do so)  
> > > > 
> > > > Even when the change is to remove the exposure of hardware from the 
> > > > guest?
> > > > Before this change, if a guest looked, it had a flash, after this 
> > > > change,
> > > > if a guest looks, it doesn't.  
> > > 
> > > It's up to the relevant maintainers who know what the semantics are.
> > > FYI ACPI tables only change across a reset though.
> > > So it's a question of whether guests get confused even if this
> > > changes after a reboot.  
> > 
> > Yup, but it's still the same "machine", so a user may wonder why it
> > changed.
> 
> you can have a different firmware with the same machine type either
> and it might look differently to guest OS but don't bother versioning
> FW. APCI tables are also part of FW (but generated by QEMU), so the same
> usually rule applies.

That makes sense. However, while users of real machines agree to update
their firmware after determining what will change, or at least expect
there could be a change that they'll need to adapt to, users of virtual
machines simply reboot, getting new firmware and ACPI tables, and then
possibly new surprises. Indeed, they may have opted to use a virtual
machine precisely so they could keep the environment stable across
updates of the real machine. IOW, since we can maintain a versioned
machine, then maybe we should?

>  
> > > Versioning is generally safer, but it's a good idea to document
> > > the motivation for it.
> > >  
> > 
> > Well, in this case, we could probably push this change to old machine
> > types and nobody would notice. If a guest is using ACPI, then it must
> > be using firmware, and if they're using firmware, then they can't be
> > using the flash. So the user shouldn't care if it's there or not. The
> > only justification for the versioning is because "it's safer". If
> > people feel strongly about avoiding versioning when it's not obviously
> > necessary, then I can respin without it.
> 
> From my pov if it doesn't break anything don't version it,

I don't see how we can be sure that we won't break anything. Although,
in this case, we *probably* won't.

> since versioning
> adds complexity which cost time during review, so it would be nicer to 
> reviewers
> and to future yourself if you can help to keep it as simple as possible.

I agree with all of that.

> 
> In this particular case I'd drop versioning.
>

So it sounds to me like we have some flexibility in our versioned machine
maintenance. We can choose to forgo the usual compat code when the risk is
deemed low enough. And, if somebody screams, we can always fix it later.
I can live with that. I'll go ahead and respin without the versioning.

Thanks,
drew