Re: [Xen-devel] [PATCH v3] vpci: honor read-only devices

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.09.2019 14:51, Roger Pau Monné  wrote:
> On Tue, Sep 03, 2019 at 02:16:52PM +0200, Jan Beulich wrote:
>> On 03.09.2019 12:14, Roger Pau Monne wrote:
>>> Don't allow the hardware domain write access the PCI config space of
>>> devices marked as read-only.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>> ---
>>> Changes since v2:
>>>  - Fix test harness.
>>>  - Do the RO check before the ownership one.
>>>
>>> Changes since v1:
>>>  - Change the approach and allow full read access, while limiting
>>>    write access to devices marked RO.
>>> ---
>>>  tools/tests/vpci/emul.h | 3 +++
>>>  xen/drivers/vpci/vpci.c | 5 +++++
>>>  2 files changed, 8 insertions(+)
>>>
>>> diff --git a/tools/tests/vpci/emul.h b/tools/tests/vpci/emul.h
>>> index 5d47544bf7..2e1d3057c9 100644
>>> --- a/tools/tests/vpci/emul.h
>>> +++ b/tools/tests/vpci/emul.h
>>> @@ -92,6 +92,9 @@ typedef union {
>>>  #define xfree(p) free(p)
>>>  
>>>  #define pci_get_pdev_by_domain(...) &test_pdev
>>> +#define pci_get_ro_map(...) NULL
>>> +
>>> +#define test_bit(...) false
>>
>> The latter seems rather dangerous to me, as a further addition of
>> test_bit() would silently build fine, but possibly produce a non-
>> working binary. But you're the defacto maintainer of this
>> harness, so if you believe it's fine so be it. (If even
>> xenpaging is considered "fine" to include xc_bitops.h, I wonder
>> if this harness couldn't do so too. And then there are three
>> test_bit() definitions overall under tools/ - I wonder if those
>> couldn't be consolidated into a single, universally usable one.)
> 
> One option would be to turn test_bit into assert(0) which should work
> for the current usage, since test_bit shouldn't be called given the
> current code and will trigger if it actually gets used. Would you be
> fine with merging the chunk below into the current patch?

That's marginally better, but not enough for my taste. IIRC under
tools/ we can't rely on DCE, and hence declaring (but not defining)
test_bit() isn't an option either.

Anyway, as said, I won't object to whatever the tool stack
maintainers are willing to give an ack for.

> I would like to avoid including xc_bitops.h, since the xenpaging
> Makefile already contains a comment regarding the wrong usage of libxc
> internals.

Right, and I wouldn't have dared to suggest this for something that
isn't just a test binary.

Jan

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