Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Gleb Natapov]

On Thu, Mar 14, 2013 at 05:13:54PM +0100, Paolo Bonzini wrote:
> Il 14/03/2013 16:59, Gleb Natapov ha scritto:
> > On Thu, Mar 14, 2013 at 04:50:40PM +0100, Paolo Bonzini wrote:
> >> Il 14/03/2013 15:23, Gleb Natapov ha scritto:
> >>> On Thu, Mar 14, 2013 at 03:05:22PM +0100, Paolo Bonzini wrote:
> >>>> Il 14/03/2013 14:56, Gleb Natapov ha scritto:
> >>>>> On Thu, Mar 14, 2013 at 02:49:48PM +0100, Paolo Bonzini wrote:
> >>>>>> Il 14/03/2013 13:34, Gleb Natapov ha scritto:
> >>>>>>>> * it can be an ISA device; the interface is the I/O port and ACPI
> >>>>>>>> support is provided just for convenience of the OSPM.  In this case,
> >>>>>>>> "-device pvevent" should just add handlers for the port.  The ACPI
> >>>>>>>> support is similar to what we do for other on-board ISA devices, for
> >>>>>>>> example serial ports (the serial ports use PIIX PCI configuration
> >>>>>>>> instead of fw-cfg, but that's a minor detail).  It only needs to work
> >>>>>>>> for port 0x505, so the fw-cfg data can be a single yes/no value and 
> >>>>>>>> only
> >>>>>>>> the _STA method needs patching.  See piix4_pm_machine_ready in
> >>>>>>>> hw/acpi_piix4.c.
> >>>>>>>
> >>>>>>> Again I think there is a big difference between well knows device and
> >>>>>>> PV devices that we add at random location. And if we make the later
> >>>>>>> configurable i.e it may or may not be present and location where it is
> >>>>>>> present can be changed then we better not make a guest to do guesses.
> >>>>>>
> >>>>>> No guesses here on part of the guest, and no probing in the firmware
> >>>>>> two.  The same number is hard-coded in QEMU and the DSDT, which go in
> >>>>>> pairs anyway, but _not_ in the guest kernel (also thanks to Hu's nice
> >>>>>> trick with the methods).
> >>>>>
> >>>>> That's the problem. The number is not hard coded in QEMU only DSDT.
> >>>>
> >>>> It is hard-coded where the board creates it, or at least as the default
> >>>> value of the qdev property.
> >>>
> >>> Default value that can be changes is not hard coded.
> >>> Why do you allow change in one place, but not the other?
> >>
> >> I'm just following the model of other ISA devices, I don't think there's
> >> any difference in this respect between well-known and pv devices (also
> >> because in the end all modern guests will use ACPI to discover even
> >> well-known devices).
> >>
> > We are not there yet :)
> 
> Kind of... Windows will hide serial ports that return not-present for
> _STA, for example.  Linux will just hide the PNPxxxx path and present it
> under /sys/bus/platform instead.
> 
> >> The board hardcodes 0x505 for pvpanic just like it hardcodes 0x3f8 for
> >> serial ports.
> >>
> >>>>> If you hard code it in QEMU (make it non configurable) and make device 
> >>>>> mandatory
> >>>>> static DSDT make sense if provided by QEMU.
> >>>>
> >>>> You cannot make it mandatory due to versioned machine types, but my plan
> >>>> would be to make it mandatory on "pc" and "pc-1.5".  For that plan it
> >>>> makes sense to have a static DSDT.  Sorry if it was unclear.
> >>>
> >>> And then you will have to have different DSDT for pre pc-1.5. Dynamic
> >>> patching solves exactly that problem.
> >>
> >> Yes, but it's enough to patch _STA.  Easier in both QEMU and the BIOS.
> >>
> > Yes, if you do not allow changing IO port patching _STA is enough, but
> > if you already patching it is easy to patch both.
> > 
> >>>>>> I think it's a nice compromise.
> >>
> >> ^^^ This still holds. :)
> > If we would have found a reasonable way to go without patching at all
> > then it would have been worthwhile to consider compromises, but if
> > patching is inevitable I honestly do not see big difference between
> > patching one place or two.
> 
> Hmm... can you do something like
> 
>         Name(PORT, 0xAAAA)
>         OperationRegion(PEOR, SystemIO, PORT, 0x01)
>         Field(PEOR, ByteAcc, NoLock, Preserve) {
>             PEPT,   8,
>         }
> 
> ? i.e. use a Name inside an OperationRegion?
> 
> If so, then we can patch 0xAAAA to zero for not-present and the port for
> present and indeed patch a single place.
> 
> If we have to patch 0x505 all over the place there's an advantage in
> patching _STA only.  But if we can do the above it's a bit cleaner to
> use the port for the patched value, indeed.
> 
And patching Name() is as simple as putting
ACPI_EXTRACT_NAME_WORD_CONST above it.

> >> We don't fail machine creation if someone wants to place a serial port
> >> at 0x5678.  With ISA it's basically garbage-in, garbage-out, I don't see
> >> a reason to make pvpanic special in this respect.
> >>
> > Fine with me. That was just a suggestion. I thought we had singleton
> > qdev flag.
> 
> We have no_user, but it's broken and not exactly a match for what you
> want here.
> 
> Paolo

--
                        Gleb.