| Message ID | 20220204063459.680961-5-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 3 | expand |
On 04.02.2022 07:34, Oleksandr Andrushchenko wrote: > A guest can read and write those registers which are not emulated and > have no respective vPCI handlers, so it can access the HW directly. I don't think this describes the present situation. Or did I miss where devices can actually be exposed to guests already, despite much of the support logic still missing? > In order to prevent a guest from reads and writes from/to the unhandled > registers make sure only hardware domain can access HW directly and restrict > guests from doing so. Tangential question: Going over the titles of the remaining patches I notice patch 6 is going to deal with BAR accesses. But (going just from the titles) I can't spot anywhere that vendor and device IDs would be exposed to guests. Yet that's the first thing guests will need in order to actually recognize devices. As said before, allowing guests access to such r/o fields is quite likely going to be fine. > --- a/xen/drivers/vpci/vpci.c > +++ b/xen/drivers/vpci/vpci.c > @@ -215,11 +215,15 @@ int vpci_remove_register(struct vpci *vpci, unsigned int offset, > } > > /* Wrappers for performing reads/writes to the underlying hardware. */ > -static uint32_t vpci_read_hw(pci_sbdf_t sbdf, unsigned int reg, > +static uint32_t vpci_read_hw(bool is_hwdom, pci_sbdf_t sbdf, unsigned int reg, > unsigned int size) Was the passing around of a boolean the consensus which was reached? Personally I'd fine it more natural if the two functions checked current->domain themselves. Jan
On 04.02.22 16:11, Jan Beulich wrote: > On 04.02.2022 07:34, Oleksandr Andrushchenko wrote: >> A guest can read and write those registers which are not emulated and >> have no respective vPCI handlers, so it can access the HW directly. > I don't think this describes the present situation. Or did I miss where > devices can actually be exposed to guests already, despite much of the > support logic still missing? No, they are not exposed yet and you know that. I will update the commit message > >> In order to prevent a guest from reads and writes from/to the unhandled >> registers make sure only hardware domain can access HW directly and restrict >> guests from doing so. > Tangential question: Going over the titles of the remaining patches I > notice patch 6 is going to deal with BAR accesses. But (going just > from the titles) I can't spot anywhere that vendor and device IDs > would be exposed to guests. Yet that's the first thing guests will need > in order to actually recognize devices. As said before, allowing guests > access to such r/o fields is quite likely going to be fine. Agree, I was thinking about adding such a patch to allow IDs, but finally decided not to add more to this series. Again, the whole thing is not working yet and for the development this patch can/needs to be reverted. So, either we implement IDs or not this doesn't change anything with this respect > >> --- a/xen/drivers/vpci/vpci.c >> +++ b/xen/drivers/vpci/vpci.c >> @@ -215,11 +215,15 @@ int vpci_remove_register(struct vpci *vpci, unsigned int offset, >> } >> >> /* Wrappers for performing reads/writes to the underlying hardware. */ >> -static uint32_t vpci_read_hw(pci_sbdf_t sbdf, unsigned int reg, >> +static uint32_t vpci_read_hw(bool is_hwdom, pci_sbdf_t sbdf, unsigned int reg, >> unsigned int size) > Was the passing around of a boolean the consensus which was reached? Was this patch committed yet? > Personally I'd fine it more natural if the two functions checked > current->domain themselves. This is also possible, but I would like to hear Roger's view on this as well I am fine either way > > Jan > Thank you, Oleksandr
On 04.02.22 16:24, Oleksandr Andrushchenko wrote: > > On 04.02.22 16:11, Jan Beulich wrote: >> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote: >>> A guest can read and write those registers which are not emulated and >>> have no respective vPCI handlers, so it can access the HW directly. >> I don't think this describes the present situation. Or did I miss where >> devices can actually be exposed to guests already, despite much of the >> support logic still missing? > No, they are not exposed yet and you know that. > I will update the commit message BTW, all this work is about adding vpci for guests and of course this is not going to be enabled right away. I would like to hear the common acceptable way of documenting such things: either we just say something like "A guest can read and write" elsewhere or we need to invent something neutral not directly mentioning what the change does. With the later it all seems a bit confusing IMO as we do know what we are doing and for what reason: enable vpci for guests >>> In order to prevent a guest from reads and writes from/to the unhandled >>> registers make sure only hardware domain can access HW directly and restrict >>> guests from doing so. >> Tangential question: Going over the titles of the remaining patches I >> notice patch 6 is going to deal with BAR accesses. But (going just >> from the titles) I can't spot anywhere that vendor and device IDs >> would be exposed to guests. Yet that's the first thing guests will need >> in order to actually recognize devices. As said before, allowing guests >> access to such r/o fields is quite likely going to be fine. > Agree, I was thinking about adding such a patch to allow IDs, > but finally decided not to add more to this series. > Again, the whole thing is not working yet and for the development > this patch can/needs to be reverted. So, either we implement IDs > or not this doesn't change anything with this respect Roger, do you want an additional patch with IDs in v7? >>> --- a/xen/drivers/vpci/vpci.c >>> +++ b/xen/drivers/vpci/vpci.c >>> @@ -215,11 +215,15 @@ int vpci_remove_register(struct vpci *vpci, unsigned int offset, >>> } >>> >>> /* Wrappers for performing reads/writes to the underlying hardware. */ >>> -static uint32_t vpci_read_hw(pci_sbdf_t sbdf, unsigned int reg, >>> +static uint32_t vpci_read_hw(bool is_hwdom, pci_sbdf_t sbdf, unsigned int reg, >>> unsigned int size) >> Was the passing around of a boolean the consensus which was reached? > Was this patch committed yet? >> Personally I'd fine it more natural if the two functions checked >> current->domain themselves. > This is also possible, but I would like to hear Roger's view on this as well > I am fine either way Roger, what's your maintainer's preference here? Additional argument to vpci_read_hw of make it use current->domain internally? Thank you, Oleksandr
On 08.02.2022 09:00, Oleksandr Andrushchenko wrote: > On 04.02.22 16:24, Oleksandr Andrushchenko wrote: >> On 04.02.22 16:11, Jan Beulich wrote: >>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote: >>>> A guest can read and write those registers which are not emulated and >>>> have no respective vPCI handlers, so it can access the HW directly. >>> I don't think this describes the present situation. Or did I miss where >>> devices can actually be exposed to guests already, despite much of the >>> support logic still missing? >> No, they are not exposed yet and you know that. >> I will update the commit message > BTW, all this work is about adding vpci for guests and of course this > is not going to be enabled right away. > I would like to hear the common acceptable way of documenting such > things: either we just say something like "A guest can read and write" > elsewhere or we need to invent something neutral not directly mentioning > what the change does. With the later it all seems a bit confusing IMO > as we do know what we are doing and for what reason: enable vpci for guests What's the problem with describing things as they are? Code is hwdom- only right now, and you're trying to enable DomU support. Hence it's all about "would be able to", not "can". Jan
On Tue, Feb 08, 2022 at 08:00:28AM +0000, Oleksandr Andrushchenko wrote: > > On 04.02.22 16:24, Oleksandr Andrushchenko wrote: > > > > On 04.02.22 16:11, Jan Beulich wrote: > >> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote: > >>> A guest can read and write those registers which are not emulated and > >>> have no respective vPCI handlers, so it can access the HW directly. > >> I don't think this describes the present situation. Or did I miss where > >> devices can actually be exposed to guests already, despite much of the > >> support logic still missing? > > No, they are not exposed yet and you know that. > > I will update the commit message > BTW, all this work is about adding vpci for guests and of course this > is not going to be enabled right away. > I would like to hear the common acceptable way of documenting such > things: either we just say something like "A guest can read and write" > elsewhere or we need to invent something neutral not directly mentioning > what the change does. With the later it all seems a bit confusing IMO > as we do know what we are doing and for what reason: enable vpci for guests > >>> In order to prevent a guest from reads and writes from/to the unhandled > >>> registers make sure only hardware domain can access HW directly and restrict > >>> guests from doing so. > >> Tangential question: Going over the titles of the remaining patches I > >> notice patch 6 is going to deal with BAR accesses. But (going just > >> from the titles) I can't spot anywhere that vendor and device IDs > >> would be exposed to guests. Yet that's the first thing guests will need > >> in order to actually recognize devices. As said before, allowing guests > >> access to such r/o fields is quite likely going to be fine. > > Agree, I was thinking about adding such a patch to allow IDs, > > but finally decided not to add more to this series. > > Again, the whole thing is not working yet and for the development > > this patch can/needs to be reverted. So, either we implement IDs > > or not this doesn't change anything with this respect > Roger, do you want an additional patch with IDs in v7? I would expect a lot more work to be required, you need IDs and the Header type as a minimum I would say. And then in order to have something functional you will also need to handle the capabilities pointer. I'm fine for this to be added in a followup series. I think it's clear the status after this series is not going to be functional. > >>> --- a/xen/drivers/vpci/vpci.c > >>> +++ b/xen/drivers/vpci/vpci.c > >>> @@ -215,11 +215,15 @@ int vpci_remove_register(struct vpci *vpci, unsigned int offset, > >>> } > >>> > >>> /* Wrappers for performing reads/writes to the underlying hardware. */ > >>> -static uint32_t vpci_read_hw(pci_sbdf_t sbdf, unsigned int reg, > >>> +static uint32_t vpci_read_hw(bool is_hwdom, pci_sbdf_t sbdf, unsigned int reg, > >>> unsigned int size) > >> Was the passing around of a boolean the consensus which was reached? > > Was this patch committed yet? > >> Personally I'd fine it more natural if the two functions checked > >> current->domain themselves. > > This is also possible, but I would like to hear Roger's view on this as well > > I am fine either way > Roger, what's your maintainer's preference here? Additional argument > to vpci_read_hw of make it use current->domain internally? My recommendation would be to use current->domain. Handlers will always be executed in guest context, so there's no need to pass a parameter around. Thanks, Roger.
On 08.02.22 11:04, Jan Beulich wrote: > On 08.02.2022 09:00, Oleksandr Andrushchenko wrote: >> On 04.02.22 16:24, Oleksandr Andrushchenko wrote: >>> On 04.02.22 16:11, Jan Beulich wrote: >>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote: >>>>> A guest can read and write those registers which are not emulated and >>>>> have no respective vPCI handlers, so it can access the HW directly. >>>> I don't think this describes the present situation. Or did I miss where >>>> devices can actually be exposed to guests already, despite much of the >>>> support logic still missing? >>> No, they are not exposed yet and you know that. >>> I will update the commit message >> BTW, all this work is about adding vpci for guests and of course this >> is not going to be enabled right away. >> I would like to hear the common acceptable way of documenting such >> things: either we just say something like "A guest can read and write" >> elsewhere or we need to invent something neutral not directly mentioning >> what the change does. With the later it all seems a bit confusing IMO >> as we do know what we are doing and for what reason: enable vpci for guests > What's the problem with describing things as they are? Code is hwdom- > only right now, and you're trying to enable DomU support. Hence it's > all about "would be able to", not "can". Sounds good, will use that wording then > > Jan > > Thank you, Oleksandr
On 08.02.22 11:05, Roger Pau Monné wrote: > On Tue, Feb 08, 2022 at 08:00:28AM +0000, Oleksandr Andrushchenko wrote: >> On 04.02.22 16:24, Oleksandr Andrushchenko wrote: >>> On 04.02.22 16:11, Jan Beulich wrote: >>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote: >>>>> A guest can read and write those registers which are not emulated and >>>>> have no respective vPCI handlers, so it can access the HW directly. >>>> I don't think this describes the present situation. Or did I miss where >>>> devices can actually be exposed to guests already, despite much of the >>>> support logic still missing? >>> No, they are not exposed yet and you know that. >>> I will update the commit message >> BTW, all this work is about adding vpci for guests and of course this >> is not going to be enabled right away. >> I would like to hear the common acceptable way of documenting such >> things: either we just say something like "A guest can read and write" >> elsewhere or we need to invent something neutral not directly mentioning >> what the change does. With the later it all seems a bit confusing IMO >> as we do know what we are doing and for what reason: enable vpci for guests >>>>> In order to prevent a guest from reads and writes from/to the unhandled >>>>> registers make sure only hardware domain can access HW directly and restrict >>>>> guests from doing so. >>>> Tangential question: Going over the titles of the remaining patches I >>>> notice patch 6 is going to deal with BAR accesses. But (going just >>>> from the titles) I can't spot anywhere that vendor and device IDs >>>> would be exposed to guests. Yet that's the first thing guests will need >>>> in order to actually recognize devices. As said before, allowing guests >>>> access to such r/o fields is quite likely going to be fine. >>> Agree, I was thinking about adding such a patch to allow IDs, >>> but finally decided not to add more to this series. >>> Again, the whole thing is not working yet and for the development >>> this patch can/needs to be reverted. So, either we implement IDs >>> or not this doesn't change anything with this respect >> Roger, do you want an additional patch with IDs in v7? > I would expect a lot more work to be required, you need IDs and the > Header type as a minimum I would say. And then in order to have > something functional you will also need to handle the capabilities > pointer. > > I'm fine for this to be added in a followup series. I think it's clear > the status after this series is not going to be functional. Ok, so let's first have something and then we can extend guest's support This can go in parallel with other work on Arm which still waits for this series to be accepted > >>>>> --- a/xen/drivers/vpci/vpci.c >>>>> +++ b/xen/drivers/vpci/vpci.c >>>>> @@ -215,11 +215,15 @@ int vpci_remove_register(struct vpci *vpci, unsigned int offset, >>>>> } >>>>> >>>>> /* Wrappers for performing reads/writes to the underlying hardware. */ >>>>> -static uint32_t vpci_read_hw(pci_sbdf_t sbdf, unsigned int reg, >>>>> +static uint32_t vpci_read_hw(bool is_hwdom, pci_sbdf_t sbdf, unsigned int reg, >>>>> unsigned int size) >>>> Was the passing around of a boolean the consensus which was reached? >>> Was this patch committed yet? >>>> Personally I'd fine it more natural if the two functions checked >>>> current->domain themselves. >>> This is also possible, but I would like to hear Roger's view on this as well >>> I am fine either way >> Roger, what's your maintainer's preference here? Additional argument >> to vpci_read_hw of make it use current->domain internally? > My recommendation would be to use current->domain. Handlers will > always be executed in guest context, so there's no need to pass a > parameter around. ok, I'll use current->domain > > Thanks, Roger. > Thank you, Oleksandr
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c index cb2ababa28e3..f8a93e61c08f 100644 --- a/xen/drivers/vpci/vpci.c +++ b/xen/drivers/vpci/vpci.c @@ -215,11 +215,15 @@ int vpci_remove_register(struct vpci *vpci, unsigned int offset, } /* Wrappers for performing reads/writes to the underlying hardware. */ -static uint32_t vpci_read_hw(pci_sbdf_t sbdf, unsigned int reg, +static uint32_t vpci_read_hw(bool is_hwdom, pci_sbdf_t sbdf, unsigned int reg, unsigned int size) { uint32_t data; + /* Guest domains are not allowed to read real hardware. */ + if ( !is_hwdom ) + return ~(uint32_t)0; + switch ( size ) { case 4: @@ -260,9 +264,13 @@ static uint32_t vpci_read_hw(pci_sbdf_t sbdf, unsigned int reg, return data; } -static void vpci_write_hw(pci_sbdf_t sbdf, unsigned int reg, unsigned int size, - uint32_t data) +static void vpci_write_hw(bool is_hwdom, pci_sbdf_t sbdf, unsigned int reg, + unsigned int size, uint32_t data) { + /* Guest domains are not allowed to write real hardware. */ + if ( !is_hwdom ) + return; + switch ( size ) { case 4: @@ -322,6 +330,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size) const struct vpci_register *r; unsigned int data_offset = 0; uint32_t data = ~(uint32_t)0; + bool is_hwdom = is_hardware_domain(d); if ( !size ) { @@ -332,13 +341,13 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size) /* Find the PCI dev matching the address. */ pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn); if ( !pdev ) - return vpci_read_hw(sbdf, reg, size); + return vpci_read_hw(is_hwdom, sbdf, reg, size); spin_lock(&pdev->vpci_lock); if ( !pdev->vpci ) { spin_unlock(&pdev->vpci_lock); - return vpci_read_hw(sbdf, reg, size); + return vpci_read_hw(is_hwdom, sbdf, reg, size); } /* Read from the hardware or the emulated register handlers. */ @@ -361,7 +370,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size) { /* Heading gap, read partial content from hardware. */ read_size = r->offset - emu.offset; - val = vpci_read_hw(sbdf, emu.offset, read_size); + val = vpci_read_hw(is_hwdom, sbdf, emu.offset, read_size); data = merge_result(data, val, read_size, data_offset); data_offset += read_size; } @@ -387,7 +396,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size) if ( data_offset < size ) { /* Tailing gap, read the remaining. */ - uint32_t tmp_data = vpci_read_hw(sbdf, reg + data_offset, + uint32_t tmp_data = vpci_read_hw(is_hwdom, sbdf, reg + data_offset, size - data_offset); data = merge_result(data, tmp_data, size - data_offset, data_offset); @@ -430,6 +439,7 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size, const struct vpci_register *r; unsigned int data_offset = 0; const unsigned long *ro_map = pci_get_ro_map(sbdf.seg); + bool is_hwdom = is_hardware_domain(d); if ( !size ) { @@ -448,7 +458,7 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size, pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn); if ( !pdev ) { - vpci_write_hw(sbdf, reg, size, data); + vpci_write_hw(is_hwdom, sbdf, reg, size, data); return; } @@ -456,7 +466,7 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size, if ( !pdev->vpci ) { spin_unlock(&pdev->vpci_lock); - vpci_write_hw(sbdf, reg, size, data); + vpci_write_hw(is_hwdom, sbdf, reg, size, data); return; } @@ -479,7 +489,7 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size, if ( emu.offset < r->offset ) { /* Heading gap, write partial content to hardware. */ - vpci_write_hw(sbdf, emu.offset, r->offset - emu.offset, + vpci_write_hw(is_hwdom, sbdf, emu.offset, r->offset - emu.offset, data >> (data_offset * 8)); data_offset += r->offset - emu.offset; } @@ -498,7 +508,7 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size, if ( data_offset < size ) /* Tailing gap, write the remaining. */ - vpci_write_hw(sbdf, reg + data_offset, size - data_offset, + vpci_write_hw(is_hwdom, sbdf, reg + data_offset, size - data_offset, data >> (data_offset * 8)); }