| Message ID | 1543407172-16175-8-git-send-email-peng.hao2@zte.com.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add pvpanic mmio support | expand |
On Wed, 28 Nov 2018 at 03:50, Peng Hao <peng.hao2@zte.com.cn> wrote: > > Prepare for pvpanic-mmio configure interface. > > Signed-off-by: Peng Hao <peng.hao2@zte.com.cn> > --- > hw/arm/sysbus-fdt.c | 2 ++ > hw/arm/virt.c | 2 ++ > hw/misc/pvpanic.c | 11 +++++++++-- > 3 files changed, 13 insertions(+), 2 deletions(-) This looks rather odd. The sysbus-fdt.c code is for handling devices that we must pass through from the host system (including pulling in device tree fragments from the host). The pvpanic device isn't a host pass through device so I don't think it should be touching that code at all. It's just a simple MMIO device. I suspect the reason you've done this is that you're trying to get "-device pvpanic" to work on the command line. I recommend not trying to get that to work. MMIO devices are not pluggable devices, and -device is not expected to work with them. thanks -- PMM
On Fri, 30 Nov 2018 at 15:56, Peter Maydell <peter.maydell@linaro.org> wrote: > I suspect the reason you've done this is that you're > trying to get "-device pvpanic" to work on the command > line. I recommend not trying to get that to work. > MMIO devices are not pluggable devices, and -device > is not expected to work with them. If you do want a pluggable pvpanic device for the virt board then you should implement it as a PCI device, incidentally. thanks -- PMM
On Fri, Nov 30, 2018 at 03:57:13PM +0000, Peter Maydell wrote: > On Fri, 30 Nov 2018 at 15:56, Peter Maydell <peter.maydell@linaro.org> wrote: > > I suspect the reason you've done this is that you're > > trying to get "-device pvpanic" to work on the command > > line. I recommend not trying to get that to work. > > MMIO devices are not pluggable devices, and -device > > is not expected to work with them. > > If you do want a pluggable pvpanic device for the virt > board then you should implement it as a PCI device, > incidentally. > We'd have to allocate it a PCI device ID, but I guess that's OK as there are plenty of IDs left for 1b36. I'm not sure it's worth it though. Phil asked that this device by user creatable, but maybe that's not necessary. Maybe we just need a mach-virt compat boolean and then to always provide this device for 4.0 and later machines? Thanks, drew
On Fri, 30 Nov 2018 at 16:14, Andrew Jones <drjones@redhat.com> wrote: > > On Fri, Nov 30, 2018 at 03:57:13PM +0000, Peter Maydell wrote: > > On Fri, 30 Nov 2018 at 15:56, Peter Maydell <peter.maydell@linaro.org> wrote: > > > I suspect the reason you've done this is that you're > > > trying to get "-device pvpanic" to work on the command > > > line. I recommend not trying to get that to work. > > > MMIO devices are not pluggable devices, and -device > > > is not expected to work with them. > > > > If you do want a pluggable pvpanic device for the virt > > board then you should implement it as a PCI device, > > incidentally. > > > > We'd have to allocate it a PCI device ID, but I guess that's OK as > there are plenty of IDs left for 1b36. I'm not sure it's worth it > though. Phil asked that this device by user creatable, but maybe > that's not necessary. Maybe we just need a mach-virt compat boolean > and then to always provide this device for 4.0 and later machines? Yes, if it's just an mmio device then we should either: * default to providing it, with the usual flag to say "don't create in older virt board versions", and also a machine property to disable it manually (like we do with the ITS) * default to not providing it at all, and have a machine property to enable it (like we do with the IOMMU) Which is all doable, but every time we do that it makes the virt board code that extra bit more complicated (we have half a dozen machine properties on it already). This kind of thing is why a PCI device is cleaner -- it just works on any machine with a PCI controller, it by default is something that the user can provide if they want to and not if they don't, and it's not a custom UI that management layers might need to special-case. The guest does need to do a bit of PCI probing and setup initially, but then it can just leave the MMIO BAR permanently mapped and write to that address when the guest panics. thanks -- PMM
>On Wed, 28 Nov 2018 at 03:50, Peng Hao <peng.hao2@zte.com.cn> wrote: >> >> Prepare for pvpanic-mmio configure interface. >> >> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn> >> --- >> hw/arm/sysbus-fdt.c | 2 ++ >> hw/arm/virt.c | 2 ++ >> hw/misc/pvpanic.c | 11 +++++++++-- >> 3 files changed, 13 insertions(+), 2 deletions(-) > >This looks rather odd. The sysbus-fdt.c code is for >handling devices that we must pass through from the >host system (including pulling in device tree fragments >from the host). The pvpanic device isn't a host pass >through device so I don't think it should be touching >that code at all. It's just a simple MMIO device. > >I suspect the reason you've done this is that you're >trying to get "-device pvpanic" to work on the command >line. I recommend not trying to get that to work. >MMIO devices are not pluggable devices, and -device >is not expected to work with them. > yes, I use -device pvpanic-mmio. Thank you for pointing this out. I also felt a little odd when I realized it. But I didn't think about the reason. I think I can use a way like iommu, and I've thought about it in doing my current way. thanks. >thanks >-- PMM
>On Fri, 30 Nov 2018 at 16:14, Andrew Jones <drjones@redhat.com> wrote: >> >> On Fri, Nov 30, 2018 at 03:57:13PM +0000, Peter Maydell wrote: >> > On Fri, 30 Nov 2018 at 15:56, Peter Maydell <peter.maydell@linaro.org> wrote: >> > > I suspect the reason you've done this is that you're >> > > trying to get "-device pvpanic" to work on the command >> > > line. I recommend not trying to get that to work. >> > > MMIO devices are not pluggable devices, and -device >> > > is not expected to work with them. >> > >> > If you do want a pluggable pvpanic device for the virt >> > board then you should implement it as a PCI device, >> > incidentally. >> > >> >> We'd have to allocate it a PCI device ID, but I guess that's OK as >> there are plenty of IDs left for 1b36. I'm not sure it's worth it >> though. Phil asked that this device by user creatable, but maybe >> that's not necessary. Maybe we just need a mach-virt compat boolean >> and then to always provide this device for 4.0 and later machines? > >Yes, if it's just an mmio device then we should either: >* default to providing it, with the usual flag to say "don't create >in older virt board versions", and also a machine property to >disable it manually (like we do with the ITS) >* default to not providing it at all, and have a machine >property to enable it (like we do with the IOMMU) >Which is all doable, but every time we do that it makes the >virt board code that extra bit more complicated (we have >half a dozen machine properties on it already). > >This kind of thing is why a PCI device is cleaner -- it just >works on any machine with a PCI controller, it by default is >something that the user can provide if they want to and not if >they don't, and it's not a custom UI that management layers >might need to special-case. The guest does need to do a bit >of PCI probing and setup initially, but then it can just leave >the MMIO BAR permanently mapped and write to that address >when the guest panics. > Usually, it doesn't matter if there is this pvpanic device or not, so I haven't considered configurability from the beginning. I didn't expect you to strongly recommend the use of PCI devices. Doing so is not just going to roll back some of the code in the kernel... I'll think about your suggestions. thanks. >thanks -- PMM
On Sat, 1 Dec 2018 at 09:28, <peng.hao2@zte.com.cn> wrote: > Peter Maydell wrote: > >This kind of thing is why a PCI device is cleaner -- it just > >works on any machine with a PCI controller, it by default is > >something that the user can provide if they want to and not if > >they don't, and it's not a custom UI that management layers > >might need to special-case. The guest does need to do a bit > >of PCI probing and setup initially, but then it can just leave > >the MMIO BAR permanently mapped and write to that address > >when the guest panics. > Usually, it doesn't matter if there is this pvpanic device or not, so I > haven't considered configurability from the beginning. It matters to people who don't want it, because it increases the attack surface presented to the guest. > I didn't expect you to strongly recommend the use of PCI devices. > Doing so is not just going to roll back some of the code in the kernel... > I'll think about your suggestions. My view on this hasn't changed since October: https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg03861.html if this really really needs to be an MMIO device then it can be done, but a PCI device is a much cleaner interface which is more widely usable than just the virt board. thanks -- PMM
diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c index ad698d4..34577f3 100644 --- a/hw/arm/sysbus-fdt.c +++ b/hw/arm/sysbus-fdt.c @@ -38,6 +38,7 @@ #include "hw/vfio/vfio-amd-xgbe.h" #include "hw/display/ramfb.h" #include "hw/arm/fdt.h" +#include "hw/misc/pvpanic.h" /* * internal struct that contains the information to create dynamic @@ -459,6 +460,7 @@ static const BindingEntry bindings[] = { VFIO_PLATFORM_BINDING("amd,xgbe-seattle-v1a", add_amd_xgbe_fdt_node), #endif TYPE_BINDING(TYPE_RAMFB_DEVICE, no_fdt_node), + TYPE_BINDING(TYPE_PVPANIC_MMIO, no_fdt_node), TYPE_BINDING("", NULL), /* last element */ }; diff --git a/hw/arm/virt.c b/hw/arm/virt.c index f2cb5de..1fd5941 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -59,6 +59,7 @@ #include "qapi/visitor.h" #include "standard-headers/linux/input.h" #include "hw/arm/smmuv3.h" +#include "hw/misc/pvpanic.h" #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \ static void virt_##major##_##minor##_class_init(ObjectClass *oc, \ @@ -1783,6 +1784,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data) machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_CALXEDA_XGMAC); machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_AMD_XGBE); machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE); + machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PVPANIC_MMIO); machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_PLATFORM); mc->block_default_type = IF_VIRTIO; mc->no_cdrom = 1; diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c index c9382a8..b6b5c89 100644 --- a/hw/misc/pvpanic.c +++ b/hw/misc/pvpanic.c @@ -67,7 +67,7 @@ typedef struct PVPanicISAState { typedef struct PVPanicMMIOState { SysBusDevice parent_obj; /*<private>*/ - + uint32_t base; /* public */ MemoryRegion mr; } PVPanicMMIOState; @@ -151,10 +151,17 @@ static void pvpanic_mmio_initfn(Object *obj) sysbus_init_mmio(sbd, &s->mr); } +static Property pvpanic_mmio_properties[] = { + DEFINE_PROP_UINT32("mmio", PVPanicMMIOState, base, 0x09070000), + DEFINE_PROP_END_OF_LIST(), +}; + static void pvpanic_mmio_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); - + + dc->user_creatable = true; + dc->props = pvpanic_mmio_properties; set_bit(DEVICE_CATEGORY_MISC, dc->categories); }
Prepare for pvpanic-mmio configure interface. Signed-off-by: Peng Hao <peng.hao2@zte.com.cn> --- hw/arm/sysbus-fdt.c | 2 ++ hw/arm/virt.c | 2 ++ hw/misc/pvpanic.c | 11 +++++++++-- 3 files changed, 13 insertions(+), 2 deletions(-)