Re: [Xen-devel] [PATCH v2 2/2] ns16550: enable use of PCI MSI

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.11.18 at 13:40, <roger.pau@xxxxxxxxxx> wrote:
> On Fri, Nov 30, 2018 at 01:52:39AM -0700, Jan Beulich wrote:
>> >>> On 29.11.18 at 18:33, <roger.pau@xxxxxxxxxx> wrote:
>> > On Mon, Oct 01, 2018 at 10:26:05AM -0600, Jan Beulich wrote:
>> >> --- a/xen/arch/x86/msi.c
>> >> +++ b/xen/arch/x86/msi.c
>> >> @@ -742,6 +742,16 @@ static int msi_capability_init(struct pc
>> >>  
>> >>      *desc = entry;
>> >>      /* Restore the original MSI enabled bits  */
>> >> +    if ( !hardware_domain )
>> > 
>> > Wouldn't it be better to assign the device to dom_xen (pdev->domain =
>> > dom_xen), and then check if the owner is dom_xen here?
>> 
>> I'm not sure this couldn't be wrong in the general case (and we
>> sit on a generic code path here): It depends on whether Dom0
>> can modify the device's config space, and I wouldn't want to
>> (here) introduce a connection between dom_xen ownership and
>> whether Dom0 can control INTX. The comment below here is
>> specifically worded to the effect of why I use hardware_domain
>> here.
> 
> Well, I think Dom0 shouldn't be allowed to interact with devices owned
> by dom_xen. That being set, at least the current vPCI code will allow
> PVH Dom0 to do so by passing through any accesses to registers not
> explicitly handled by vPCI.

Well, the r/o devices we expose don't match this desire of yours.
I also think that we ought to be very careful with completely hiding
devices from Dom0, as there may be implications between the
existence of devices (whether such implications are always
conceptually correct would be an orthogonal question).

>> If we ever get into the situation of wanting to enable MSI on an
>> internally used device _after_ Dom0 has started, this would need
>> careful re-considering.
> 
> OK, I'm fine with this. Maybe using system_state would be clearer to
> note that this code path is only to be used during early boot?

I don't think so, as a hardware domain cannot possibly fail to
exist at normal runtime. And system_state gets set to
SYS_STATE_active too late for my taste, while tying the logic
to SYS_STATE_smp_boot seems inappropriate to me.

>> > Or at the point where this is called from the serial console driver is
>> > too early for dom_xen to exist?
>> 
>> ns16550_init_postirq() is where both MSI setup and hiding of the
>> device happen, so in principle this would seem to be possible for
>> the specific case of a serial card.
> 
> IMO it's clear from a conceptual PoV to check against the ownership of
> the device rather than the system state at the point of the function
> call. Devices assigned to Dom0 use the split model, devices assigned
> to Xen don't.

Except aforementioned r/o devices, which Dom0 can see but not
(generally) fiddle with.

>> >> --- a/xen/drivers/pci/pci.c
>> >> +++ b/xen/drivers/pci/pci.c
>> >> @@ -115,6 +115,21 @@ int pci_find_next_ext_capability(int seg
>> >>      return 0;
>> >>  }
>> >>  
>> >> +void pci_intx(const struct pci_dev *pdev, bool_t enable)
>> > 
>> > Please use bool.
>> 
>> See how old this patch is. V1 was posted long before bool came into
>> existence, and I had refrained from posting v2 until I actually had a
>> device where MSI would indeed function (the first two I tried this
>> with claimed to be MSI capable, but no interrupts ever surfaced
>> when MSI was enabled on them, yet I couldn't be sure the code
>> was doing something wrong). Obviously I then forgot to switch this,
>> which I've now done.
> 
> Thanks.
> 
> With the bool change:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks!

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel