Re: [PATCH] pvpanic: Advertise the PVPANIC_CRASHLOADED event support

[Alejandro Jimenez]

(CC Maciej)

Hi Paolo,

Thank you for fixing the patch. One comment below..

On 11/9/2020 9:33 AM, Paolo Bonzini wrote:
> Advertise both types of events as supported when the guest OS
> queries the pvpanic device.  Currently only PVPANIC_PANICKED is
> exposed; PVPANIC_CRASHLOADED must also be advertised, but only on
> new machine types.
>
> Fixes: 7dc58deea79a ("pvpanic: implement crashloaded event handling")
> Reported-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/core/machine.c | 1 +
>   hw/misc/pvpanic.c | 5 ++++-
>   2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 98b87f76cb..d0408049b5 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -35,6 +35,7 @@ GlobalProperty hw_compat_5_1[] = {
>       { "virtio-blk-device", "num-queues", "1"},
>       { "virtio-scsi-device", "num_queues", "1"},
>       { "nvme", "use-intel-id", "on"},
> +    { "pvpanic", "events", "1"}, /* PVPANIC_PANICKED */
>   };
>   const size_t hw_compat_5_1_len = G_N_ELEMENTS(hw_compat_5_1);
>   
> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> index 598d5471a4..35d6797831 100644
> --- a/hw/misc/pvpanic.c
> +++ b/hw/misc/pvpanic.c
> @@ -61,12 +61,14 @@ struct PVPanicState {
>   
>       MemoryRegion io;
>       uint16_t ioport;
> +    uint8_t events;
>   };
>   
>   /* return supported events on read */
>   static uint64_t pvpanic_ioport_read(void *opaque, hwaddr addr, unsigned size)
>   {
> -    return PVPANIC_PANICKED;
> +    PVPanicState *pvp = opaque;
> +    return pvp->events;
>   }
>   
>   static void pvpanic_ioport_write(void *opaque, hwaddr addr, uint64_t val,
> @@ -112,6 +114,7 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error 
> **errp)
>   
>   static Property pvpanic_isa_properties[] = {
>       DEFINE_PROP_UINT16(PVPANIC_IOPORT_PROP, PVPanicState, ioport, 0x505),
> +    DEFINE_PROP_UINT8("events", PVPanicState, events, PVPANIC_PANICKED | 
> PVPANIC_CRASHLOADED),
>       DEFINE_PROP_END_OF_LIST(),
>   };
One side effect of defining the "events" property (as Maciej pointed out 
to me), is that we are able to overwrite it with an alternative value in 
the cmdline. e.g.

-device pvpanic,events=<event mask>

This can be used to force QEMU to report a set of events different than 
what it actually supports to a guest OS reading from the pvpanic device. 
I assume you are not concerned about this scenario, since it would have 
to be deliberately triggered, and even then it would not cause any 
serious issues, correct?

Assuming the above is not a problem:
Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>