| Message ID | 1599769330-17656-2-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IOREQ feature (+ virtio-mmio) on Arm | expand |
On 10.09.2020 22:21, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > As a lot of x86 code can be re-used on Arm later on, this patch > prepares IOREQ support before moving to the common code. This way > we will get almost a verbatim copy for a code movement. > > This support is going to be used on Arm to be able run device > emulator outside of Xen hypervisor. This is all fine, but you should then go on and explain what you're doing, and why (at which point it may become obvious that it would be more helpful to split this into a couple of steps). In particular something as suspicious as ... > Changes RFC -> V1: > - new patch, was split from: > "[RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common" > - fold the check of p->type into hvm_get_ioreq_server_range_type() > and make it return success/failure > - remove relocate_portio_handler() call from arch_hvm_ioreq_destroy() > in arch/x86/hvm/ioreq.c ... this (see below). > --- a/xen/arch/x86/hvm/ioreq.c > +++ b/xen/arch/x86/hvm/ioreq.c > @@ -170,6 +170,29 @@ static bool hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, ioreq_t *p) > return true; > } > > +bool arch_handle_hvm_io_completion(enum hvm_io_completion io_completion) Do we need "handle" in here? Without it, I'd also not have to ask about moving hvm further to the front of the name... > @@ -836,6 +848,12 @@ int hvm_create_ioreq_server(struct domain *d, int bufioreq_handling, > return rc; > } > > +/* Called when target domain is paused */ > +int arch_hvm_destroy_ioreq_server(struct hvm_ioreq_server *s) > +{ > + return p2m_set_ioreq_server(s->target, 0, s); > +} Why return "int" when ... > @@ -855,7 +873,7 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id) > > domain_pause(d); > > - p2m_set_ioreq_server(d, 0, s); > + arch_hvm_destroy_ioreq_server(s); ... the result has been ignored anyway? Or otherwise I guess you'd want to add error handling here (but then the result of p2m_set_ioreq_server() should still get ignored, for backwards compatibility). > @@ -1215,8 +1233,7 @@ void hvm_destroy_all_ioreq_servers(struct domain *d) > struct hvm_ioreq_server *s; > unsigned int id; > > - if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) ) > - return; > + arch_hvm_ioreq_destroy(d); So the call to relocate_portio_handler() simply goes away. No replacement, no explanation? > @@ -1239,19 +1256,15 @@ void hvm_destroy_all_ioreq_servers(struct domain *d) > spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock); > } > > -struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d, > - ioreq_t *p) > +int hvm_get_ioreq_server_range_type(struct domain *d, > + ioreq_t *p, At least p, but perhaps also d can gain const? > + uint8_t *type, > + uint64_t *addr) By the name the function returns a type for a range (albeit without it being clear where the two bounds of such a range actually live). By the implementation is looks more like you mean "range_and_type", albeit still without there really being a range passed back to the caller. Therefore I think I need some clarification on what's intended before even being able to suggest something. From ... > +struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d, > + ioreq_t *p) > +{ > + struct hvm_ioreq_server *s; > + uint8_t type; > + uint64_t addr; > + unsigned int id; > + > + if ( hvm_get_ioreq_server_range_type(d, p, &type, &addr) ) > + return NULL; ... this use - maybe hvm_ioreq_server_get_type_addr() (albeit I don't like this name very much)? > @@ -1351,7 +1378,7 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p) > pg = iorp->va; > > if ( !pg ) > - return X86EMUL_UNHANDLEABLE; > + return IOREQ_IO_UNHANDLED; At this example - why the IO infix, duplicating the prefix? I'd suggest to either drop it (if the remaining identifiers are deemed unambiguous enough) or use e.g. IOREQ_STATUS_*. > @@ -1515,11 +1542,21 @@ static int hvm_access_cf8( > return X86EMUL_UNHANDLEABLE; > } > > +void arch_hvm_ioreq_init(struct domain *d) > +{ > + register_portio_handler(d, 0xcf8, 4, hvm_access_cf8); > +} > + > +void arch_hvm_ioreq_destroy(struct domain *d) > +{ > + > +} Stray blank line? Jan
On 14.09.20 16:52, Jan Beulich wrote: Hi Jan > On 10.09.2020 22:21, Oleksandr Tyshchenko wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> As a lot of x86 code can be re-used on Arm later on, this patch >> prepares IOREQ support before moving to the common code. This way >> we will get almost a verbatim copy for a code movement. >> >> This support is going to be used on Arm to be able run device >> emulator outside of Xen hypervisor. > This is all fine, but you should then go on and explain what you're > doing, and why (at which point it may become obvious that it would > be more helpful to split this into a couple of steps). Got it. Will add an explanation. > In particular > something as suspicious as ... > >> Changes RFC -> V1: >> - new patch, was split from: >> "[RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common" >> - fold the check of p->type into hvm_get_ioreq_server_range_type() >> and make it return success/failure >> - remove relocate_portio_handler() call from arch_hvm_ioreq_destroy() >> in arch/x86/hvm/ioreq.c > ... this (see below). > >> --- a/xen/arch/x86/hvm/ioreq.c >> +++ b/xen/arch/x86/hvm/ioreq.c >> @@ -170,6 +170,29 @@ static bool hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, ioreq_t *p) >> return true; >> } >> >> +bool arch_handle_hvm_io_completion(enum hvm_io_completion io_completion) > Do we need "handle" in here? Without it, I'd also not have to ask about > moving hvm further to the front of the name... For me without "handle" it will sound a bit confusing because of the enum type which has a similar name but without "arch" prefix: bool arch_hvm_io_completion(enum hvm_io_completion io_completion) Shall I then move "hvm" to the front of the name here? > >> @@ -836,6 +848,12 @@ int hvm_create_ioreq_server(struct domain *d, int bufioreq_handling, >> return rc; >> } >> >> +/* Called when target domain is paused */ >> +int arch_hvm_destroy_ioreq_server(struct hvm_ioreq_server *s) >> +{ >> + return p2m_set_ioreq_server(s->target, 0, s); >> +} > Why return "int" when ... > >> @@ -855,7 +873,7 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id) >> >> domain_pause(d); >> >> - p2m_set_ioreq_server(d, 0, s); >> + arch_hvm_destroy_ioreq_server(s); > ... the result has been ignored anyway? Or otherwise I guess you'd > want to add error handling here (but then the result of > p2m_set_ioreq_server() should still get ignored, for backwards > compatibility). I didn't have a plan to add error handling here. Agree, will make arch_hvm_destroy_ioreq_server() returning void. > >> @@ -1215,8 +1233,7 @@ void hvm_destroy_all_ioreq_servers(struct domain *d) >> struct hvm_ioreq_server *s; >> unsigned int id; >> >> - if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) ) >> - return; >> + arch_hvm_ioreq_destroy(d); > So the call to relocate_portio_handler() simply goes away. No > replacement, no explanation? As I understand from the review comment on that for the RFC patch, there is no a lot of point of keeping this. Indeed, looking at the code I got the same opinion. I should have added an explanation in the commit description at least. Or shall I return the call back? > >> @@ -1239,19 +1256,15 @@ void hvm_destroy_all_ioreq_servers(struct domain *d) >> spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock); >> } >> >> -struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d, >> - ioreq_t *p) >> +int hvm_get_ioreq_server_range_type(struct domain *d, >> + ioreq_t *p, > At least p, but perhaps also d can gain const? Agree, will add. > >> + uint8_t *type, >> + uint64_t *addr) > By the name the function returns a type for a range (albeit without > it being clear where the two bounds of such a range actually live). > By the implementation is looks more like you mean "range_and_type", > albeit still without there really being a range passed back to the > caller. Therefore I think I need some clarification on what's > intended before even being able to suggest something. This function is just an attempt to split arch specific things (cf8 handling) from "common" hvm_select_ioreq_server(). > From ... > >> +struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d, >> + ioreq_t *p) >> +{ >> + struct hvm_ioreq_server *s; >> + uint8_t type; >> + uint64_t addr; >> + unsigned int id; >> + >> + if ( hvm_get_ioreq_server_range_type(d, p, &type, &addr) ) >> + return NULL; > ... this use - maybe hvm_ioreq_server_get_type_addr() (albeit I > don't like this name very much)? Yes, hvm_ioreq_server_get_type_addr() better represents what function does. > >> @@ -1351,7 +1378,7 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p) >> pg = iorp->va; >> >> if ( !pg ) >> - return X86EMUL_UNHANDLEABLE; >> + return IOREQ_IO_UNHANDLED; > At this example - why the IO infix, duplicating the prefix? I'd > suggest to either drop it (if the remaining identifiers are deemed > unambiguous enough) or use e.g. IOREQ_STATUS_*. Agree, I would prefer IOREQ_STATUS_* >> @@ -1515,11 +1542,21 @@ static int hvm_access_cf8( >> return X86EMUL_UNHANDLEABLE; >> } >> >> +void arch_hvm_ioreq_init(struct domain *d) >> +{ >> + register_portio_handler(d, 0xcf8, 4, hvm_access_cf8); >> +} >> + >> +void arch_hvm_ioreq_destroy(struct domain *d) >> +{ >> + >> +} > Stray blank line? Will remove.
On 21.09.2020 14:22, Oleksandr wrote: > On 14.09.20 16:52, Jan Beulich wrote: >> On 10.09.2020 22:21, Oleksandr Tyshchenko wrote: >>> --- a/xen/arch/x86/hvm/ioreq.c >>> +++ b/xen/arch/x86/hvm/ioreq.c >>> @@ -170,6 +170,29 @@ static bool hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, ioreq_t *p) >>> return true; >>> } >>> >>> +bool arch_handle_hvm_io_completion(enum hvm_io_completion io_completion) >> Do we need "handle" in here? Without it, I'd also not have to ask about >> moving hvm further to the front of the name... > For me without "handle" it will sound a bit confusing because of the > enum type which > has a similar name but without "arch" prefix: > bool arch_hvm_io_completion(enum hvm_io_completion io_completion) Every function handles something; there's no point including "handle" in every name. Or else we'd have handle_memset() instead of just memset(), for example. > Shall I then move "hvm" to the front of the name here? As per comments on later patches, I think we want to consider dropping hvm prefixes or infixes altogether from the functions involved here. >>> @@ -1215,8 +1233,7 @@ void hvm_destroy_all_ioreq_servers(struct domain *d) >>> struct hvm_ioreq_server *s; >>> unsigned int id; >>> >>> - if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) ) >>> - return; >>> + arch_hvm_ioreq_destroy(d); >> So the call to relocate_portio_handler() simply goes away. No >> replacement, no explanation? > As I understand from the review comment on that for the RFC patch, there > is no > a lot of point of keeping this. Indeed, looking at the code I got the > same opinion. > I should have added an explanation in the commit description at least. > Or shall I return the call back? If there's a reason to drop it (which I can't see, but I also don't recall seeing the discussion you're mentioning), then doing so should be a separate patch with suitable reasoning. In the patch here you definitely should only transform what's already there. Jan
On 21.09.20 15:31, Jan Beulich wrote: Hi > On 21.09.2020 14:22, Oleksandr wrote: >> On 14.09.20 16:52, Jan Beulich wrote: >>> On 10.09.2020 22:21, Oleksandr Tyshchenko wrote: >>>> --- a/xen/arch/x86/hvm/ioreq.c >>>> +++ b/xen/arch/x86/hvm/ioreq.c >>>> @@ -170,6 +170,29 @@ static bool hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, ioreq_t *p) >>>> return true; >>>> } >>>> >>>> +bool arch_handle_hvm_io_completion(enum hvm_io_completion io_completion) >>> Do we need "handle" in here? Without it, I'd also not have to ask about >>> moving hvm further to the front of the name... >> For me without "handle" it will sound a bit confusing because of the >> enum type which >> has a similar name but without "arch" prefix: >> bool arch_hvm_io_completion(enum hvm_io_completion io_completion) > Every function handles something; there's no point including > "handle" in every name. Or else we'd have handle_memset() > instead of just memset(), for example. Got it. Will remove "handle" here. > >> Shall I then move "hvm" to the front of the name here? > As per comments on later patches, I think we want to consider dropping > hvm prefixes or infixes altogether from the functions involved here. >>>> @@ -1215,8 +1233,7 @@ void hvm_destroy_all_ioreq_servers(struct domain *d) >>>> struct hvm_ioreq_server *s; >>>> unsigned int id; >>>> >>>> - if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) ) >>>> - return; >>>> + arch_hvm_ioreq_destroy(d); >>> So the call to relocate_portio_handler() simply goes away. No >>> replacement, no explanation? >> As I understand from the review comment on that for the RFC patch, there >> is no >> a lot of point of keeping this. Indeed, looking at the code I got the >> same opinion. >> I should have added an explanation in the commit description at least. >> Or shall I return the call back? > If there's a reason to drop it (which I can't see, but I also > don't recall seeing the discussion you're mentioning), then doing > so should be a separate patch with suitable reasoning. In the > patch here you definitely should only transform what's already > there. Sounds reasonable. Please see the comment below relocate_portio_handler() here: https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg78512.html However, I might interpret the request incorrectly.
On 21.09.2020 14:47, Oleksandr wrote: > On 21.09.20 15:31, Jan Beulich wrote: >> On 21.09.2020 14:22, Oleksandr wrote: >>> On 14.09.20 16:52, Jan Beulich wrote: >>>> On 10.09.2020 22:21, Oleksandr Tyshchenko wrote: >>>>> @@ -1215,8 +1233,7 @@ void hvm_destroy_all_ioreq_servers(struct domain *d) >>>>> struct hvm_ioreq_server *s; >>>>> unsigned int id; >>>>> >>>>> - if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) ) >>>>> - return; >>>>> + arch_hvm_ioreq_destroy(d); >>>> So the call to relocate_portio_handler() simply goes away. No >>>> replacement, no explanation? >>> As I understand from the review comment on that for the RFC patch, there >>> is no >>> a lot of point of keeping this. Indeed, looking at the code I got the >>> same opinion. >>> I should have added an explanation in the commit description at least. >>> Or shall I return the call back? >> If there's a reason to drop it (which I can't see, but I also >> don't recall seeing the discussion you're mentioning), then doing >> so should be a separate patch with suitable reasoning. In the >> patch here you definitely should only transform what's already >> there. > Sounds reasonable. Please see the comment below > relocate_portio_handler() here: > https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg78512.html > > However, I might interpret the request incorrectly. I'm afraid you do: The way you've coded it the function was a no-op. But that's because you broke the caller by not bailing from hvm_destroy_all_ioreq_servers() if relocate_portio_handler() returned false. IOW you did assume that moving the "return" statement into an inline function would have an effect on its caller(s). For questions like this one it also often helps to look at the commit introducing the construct in question (b3344bb1cae0 in this case): Chances are that the description helps, albeit I agree there are many cases (particularly the farther you get into distant past) where it isn't of much help. Jan
On 21.09.20 16:29, Jan Beulich wrote: Hi > On 21.09.2020 14:47, Oleksandr wrote: >> On 21.09.20 15:31, Jan Beulich wrote: >>> On 21.09.2020 14:22, Oleksandr wrote: >>>> On 14.09.20 16:52, Jan Beulich wrote: >>>>> On 10.09.2020 22:21, Oleksandr Tyshchenko wrote: >>>>>> @@ -1215,8 +1233,7 @@ void hvm_destroy_all_ioreq_servers(struct domain *d) >>>>>> struct hvm_ioreq_server *s; >>>>>> unsigned int id; >>>>>> >>>>>> - if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) ) >>>>>> - return; >>>>>> + arch_hvm_ioreq_destroy(d); >>>>> So the call to relocate_portio_handler() simply goes away. No >>>>> replacement, no explanation? >>>> As I understand from the review comment on that for the RFC patch, there >>>> is no >>>> a lot of point of keeping this. Indeed, looking at the code I got the >>>> same opinion. >>>> I should have added an explanation in the commit description at least. >>>> Or shall I return the call back? >>> If there's a reason to drop it (which I can't see, but I also >>> don't recall seeing the discussion you're mentioning), then doing >>> so should be a separate patch with suitable reasoning. In the >>> patch here you definitely should only transform what's already >>> there. >> Sounds reasonable. Please see the comment below >> relocate_portio_handler() here: >> https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg78512.html >> >> However, I might interpret the request incorrectly. > I'm afraid you do: The way you've coded it the function was a no-op. > But that's because you broke the caller by not bailing from > hvm_destroy_all_ioreq_servers() if relocate_portio_handler() returned > false. IOW you did assume that moving the "return" statement into an > inline function would have an effect on its caller(s). For questions > like this one it also often helps to look at the commit introducing > the construct in question (b3344bb1cae0 in this case): Chances are > that the description helps, albeit I agree there are many cases > (particularly the farther you get into distant past) where it isn't > of much help. Hmm, now it's clear to me what I did wrong. By calling relocate_portio_handler() here we don't really want to relocate something, we just use it as a flag to indicate whether we need to actually release IOREQ resources down the hvm_destroy_all_ioreq_servers(). Thank you for the explanation, it wasn't obvious to me at the beginning. But, now the question is how to do it in a correct way and retain current behavior (to not break callers)? I see two options here: 1. Place the check of relocate_portio_handler() in arch_hvm_ioreq_destroy() and make the later returning bool. The "common" hvm_destroy_all_ioreq_servers() will check for the return value and bail out if false. 2. Don't use relocate_portio_handler(), instead introduce a flag into struct hvm_domain's ioreq_server sub-structure. Personally I don't like much the option 1 and option 2 is a little bit overhead. What do you think?
On 21.09.2020 16:43, Oleksandr wrote: > On 21.09.20 16:29, Jan Beulich wrote: >> On 21.09.2020 14:47, Oleksandr wrote: >>> On 21.09.20 15:31, Jan Beulich wrote: >>>> On 21.09.2020 14:22, Oleksandr wrote: >>>>> On 14.09.20 16:52, Jan Beulich wrote: >>>>>> On 10.09.2020 22:21, Oleksandr Tyshchenko wrote: >>>>>>> @@ -1215,8 +1233,7 @@ void hvm_destroy_all_ioreq_servers(struct domain *d) >>>>>>> struct hvm_ioreq_server *s; >>>>>>> unsigned int id; >>>>>>> >>>>>>> - if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) ) >>>>>>> - return; >>>>>>> + arch_hvm_ioreq_destroy(d); >>>>>> So the call to relocate_portio_handler() simply goes away. No >>>>>> replacement, no explanation? >>>>> As I understand from the review comment on that for the RFC patch, there >>>>> is no >>>>> a lot of point of keeping this. Indeed, looking at the code I got the >>>>> same opinion. >>>>> I should have added an explanation in the commit description at least. >>>>> Or shall I return the call back? >>>> If there's a reason to drop it (which I can't see, but I also >>>> don't recall seeing the discussion you're mentioning), then doing >>>> so should be a separate patch with suitable reasoning. In the >>>> patch here you definitely should only transform what's already >>>> there. >>> Sounds reasonable. Please see the comment below >>> relocate_portio_handler() here: >>> https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg78512.html >>> >>> However, I might interpret the request incorrectly. >> I'm afraid you do: The way you've coded it the function was a no-op. >> But that's because you broke the caller by not bailing from >> hvm_destroy_all_ioreq_servers() if relocate_portio_handler() returned >> false. IOW you did assume that moving the "return" statement into an >> inline function would have an effect on its caller(s). For questions >> like this one it also often helps to look at the commit introducing >> the construct in question (b3344bb1cae0 in this case): Chances are >> that the description helps, albeit I agree there are many cases >> (particularly the farther you get into distant past) where it isn't >> of much help. > > > Hmm, now it's clear to me what I did wrong. By calling > relocate_portio_handler() here we don't really want to relocate > something, we just use it as a flag to indicate whether we need to > actually release IOREQ resources down the > hvm_destroy_all_ioreq_servers(). Thank you for the explanation, it > wasn't obvious to me at the beginning. But, now the question is how to > do it in a correct way and retain current behavior (to not break callers)? > > I see two options here: > 1. Place the check of relocate_portio_handler() in > arch_hvm_ioreq_destroy() and make the later returning bool. > The "common" hvm_destroy_all_ioreq_servers() will check for the > return value and bail out if false. > 2. Don't use relocate_portio_handler(), instead introduce a flag into > struct hvm_domain's ioreq_server sub-structure. > > > Personally I don't like much the option 1 and option 2 is a little bit > overhead. Well, 1 is what matches current behavior, so I'd advocate for you not changing the abstract model. Or else, again, change the abstract model in a separate prereq patch. Jan
Hi, On 10/09/2020 21:21, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > As a lot of x86 code can be re-used on Arm later on, this patch > prepares IOREQ support before moving to the common code. This way > we will get almost a verbatim copy for a code movement. FWIW, I agree with Jan that we need more details what and why you are going it. It would be worth considering to split in smaller patch. > > This support is going to be used on Arm to be able run device > emulator outside of Xen hypervisor. > > Signed-off-by: Julien Grall <julien.grall@arm.com> Usually the first signed-off is the author of the patch. However, this patch look quite far off from what I originally wrote. So I don't feel my signed-off-by is actually warrant here. If you want to credit me, then you can mention it in the commit message. > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > Cheers,
On 23.09.20 20:22, Julien Grall wrote: > Hi, Hi Julien > > On 10/09/2020 21:21, Oleksandr Tyshchenko wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> As a lot of x86 code can be re-used on Arm later on, this patch >> prepares IOREQ support before moving to the common code. This way >> we will get almost a verbatim copy for a code movement. > > FWIW, I agree with Jan that we need more details what and why you are > going it. It would be worth considering to split in smaller patch. ok > > >> >> This support is going to be used on Arm to be able run device >> emulator outside of Xen hypervisor. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> > > Usually the first signed-off is the author of the patch. However, this > patch look quite far off from what I originally wrote. > > So I don't feel my signed-off-by is actually warrant here. If you want > to credit me, then you can mention it in the commit message. This is related to all patches is this series. This patch series is the second attempt (the first was RFC) to make IOREQ support common and it became quite different from the initial commit. I am sorry, I got completely lost whether the particular patch in this series is close to what you originally wrote or far from, I mean whether I should retain your SoB and whether I should drop it. So in order *not to make a mistake* is such an important question, I decided to add your SoB to each patch in this series and also add a note to each patch describing where this series came from. > >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> > > Cheers, >
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c index 1cc27df..d912655 100644 --- a/xen/arch/x86/hvm/ioreq.c +++ b/xen/arch/x86/hvm/ioreq.c @@ -170,6 +170,29 @@ static bool hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, ioreq_t *p) return true; } +bool arch_handle_hvm_io_completion(enum hvm_io_completion io_completion) +{ + switch ( io_completion ) + { + case HVMIO_realmode_completion: + { + struct hvm_emulate_ctxt ctxt; + + hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs()); + vmx_realmode_emulate_one(&ctxt); + hvm_emulate_writeback(&ctxt); + + break; + } + + default: + ASSERT_UNREACHABLE(); + break; + } + + return true; +} + bool handle_hvm_io_completion(struct vcpu *v) { struct domain *d = v->domain; @@ -209,19 +232,8 @@ bool handle_hvm_io_completion(struct vcpu *v) return handle_pio(vio->io_req.addr, vio->io_req.size, vio->io_req.dir); - case HVMIO_realmode_completion: - { - struct hvm_emulate_ctxt ctxt; - - hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs()); - vmx_realmode_emulate_one(&ctxt); - hvm_emulate_writeback(&ctxt); - - break; - } default: - ASSERT_UNREACHABLE(); - break; + return arch_handle_hvm_io_completion(io_completion); } return true; @@ -836,6 +848,12 @@ int hvm_create_ioreq_server(struct domain *d, int bufioreq_handling, return rc; } +/* Called when target domain is paused */ +int arch_hvm_destroy_ioreq_server(struct hvm_ioreq_server *s) +{ + return p2m_set_ioreq_server(s->target, 0, s); +} + int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id) { struct hvm_ioreq_server *s; @@ -855,7 +873,7 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id) domain_pause(d); - p2m_set_ioreq_server(d, 0, s); + arch_hvm_destroy_ioreq_server(s); hvm_ioreq_server_disable(s); @@ -1215,8 +1233,7 @@ void hvm_destroy_all_ioreq_servers(struct domain *d) struct hvm_ioreq_server *s; unsigned int id; - if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) ) - return; + arch_hvm_ioreq_destroy(d); spin_lock_recursive(&d->arch.hvm.ioreq_server.lock); @@ -1239,19 +1256,15 @@ void hvm_destroy_all_ioreq_servers(struct domain *d) spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock); } -struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d, - ioreq_t *p) +int hvm_get_ioreq_server_range_type(struct domain *d, + ioreq_t *p, + uint8_t *type, + uint64_t *addr) { - struct hvm_ioreq_server *s; - uint32_t cf8; - uint8_t type; - uint64_t addr; - unsigned int id; + uint32_t cf8 = d->arch.hvm.pci_cf8; if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO ) - return NULL; - - cf8 = d->arch.hvm.pci_cf8; + return -EINVAL; if ( p->type == IOREQ_TYPE_PIO && (p->addr & ~3) == 0xcfc && @@ -1264,8 +1277,8 @@ struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d, reg = hvm_pci_decode_addr(cf8, p->addr, &sbdf); /* PCI config data cycle */ - type = XEN_DMOP_IO_RANGE_PCI; - addr = ((uint64_t)sbdf.sbdf << 32) | reg; + *type = XEN_DMOP_IO_RANGE_PCI; + *addr = ((uint64_t)sbdf.sbdf << 32) | reg; /* AMD extended configuration space access? */ if ( CF8_ADDR_HI(cf8) && d->arch.cpuid->x86_vendor == X86_VENDOR_AMD && @@ -1277,16 +1290,30 @@ struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d, if ( !rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) && (msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT)) ) - addr |= CF8_ADDR_HI(cf8); + *addr |= CF8_ADDR_HI(cf8); } } else { - type = (p->type == IOREQ_TYPE_PIO) ? - XEN_DMOP_IO_RANGE_PORT : XEN_DMOP_IO_RANGE_MEMORY; - addr = p->addr; + *type = (p->type == IOREQ_TYPE_PIO) ? + XEN_DMOP_IO_RANGE_PORT : XEN_DMOP_IO_RANGE_MEMORY; + *addr = p->addr; } + return 0; +} + +struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d, + ioreq_t *p) +{ + struct hvm_ioreq_server *s; + uint8_t type; + uint64_t addr; + unsigned int id; + + if ( hvm_get_ioreq_server_range_type(d, p, &type, &addr) ) + return NULL; + FOR_EACH_IOREQ_SERVER(d, id, s) { struct rangeset *r; @@ -1351,7 +1378,7 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p) pg = iorp->va; if ( !pg ) - return X86EMUL_UNHANDLEABLE; + return IOREQ_IO_UNHANDLED; /* * Return 0 for the cases we can't deal with: @@ -1381,7 +1408,7 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p) break; default: gdprintk(XENLOG_WARNING, "unexpected ioreq size: %u\n", p->size); - return X86EMUL_UNHANDLEABLE; + return IOREQ_IO_UNHANDLED; } spin_lock(&s->bufioreq_lock); @@ -1391,7 +1418,7 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p) { /* The queue is full: send the iopacket through the normal path. */ spin_unlock(&s->bufioreq_lock); - return X86EMUL_UNHANDLEABLE; + return IOREQ_IO_UNHANDLED; } pg->buf_ioreq[pg->ptrs.write_pointer % IOREQ_BUFFER_SLOT_NUM] = bp; @@ -1422,7 +1449,7 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p) notify_via_xen_event_channel(d, s->bufioreq_evtchn); spin_unlock(&s->bufioreq_lock); - return X86EMUL_OKAY; + return IOREQ_IO_HANDLED; } int hvm_send_ioreq(struct hvm_ioreq_server *s, ioreq_t *proto_p, @@ -1438,7 +1465,7 @@ int hvm_send_ioreq(struct hvm_ioreq_server *s, ioreq_t *proto_p, return hvm_send_buffered_ioreq(s, proto_p); if ( unlikely(!vcpu_start_shutdown_deferral(curr)) ) - return X86EMUL_RETRY; + return IOREQ_IO_RETRY; list_for_each_entry ( sv, &s->ioreq_vcpu_list, @@ -1478,11 +1505,11 @@ int hvm_send_ioreq(struct hvm_ioreq_server *s, ioreq_t *proto_p, notify_via_xen_event_channel(d, port); sv->pending = true; - return X86EMUL_RETRY; + return IOREQ_IO_RETRY; } } - return X86EMUL_UNHANDLEABLE; + return IOREQ_IO_UNHANDLED; } unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered) @@ -1496,7 +1523,7 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered) if ( !s->enabled ) continue; - if ( hvm_send_ioreq(s, p, buffered) == X86EMUL_UNHANDLEABLE ) + if ( hvm_send_ioreq(s, p, buffered) == IOREQ_IO_UNHANDLED ) failed++; } @@ -1515,11 +1542,21 @@ static int hvm_access_cf8( return X86EMUL_UNHANDLEABLE; } +void arch_hvm_ioreq_init(struct domain *d) +{ + register_portio_handler(d, 0xcf8, 4, hvm_access_cf8); +} + +void arch_hvm_ioreq_destroy(struct domain *d) +{ + +} + void hvm_ioreq_init(struct domain *d) { spin_lock_init(&d->arch.hvm.ioreq_server.lock); - register_portio_handler(d, 0xcf8, 4, hvm_access_cf8); + arch_hvm_ioreq_init(d); } /* diff --git a/xen/include/asm-x86/hvm/ioreq.h b/xen/include/asm-x86/hvm/ioreq.h index e2588e9..151b92b 100644 --- a/xen/include/asm-x86/hvm/ioreq.h +++ b/xen/include/asm-x86/hvm/ioreq.h @@ -55,6 +55,22 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered); void hvm_ioreq_init(struct domain *d); +int arch_hvm_destroy_ioreq_server(struct hvm_ioreq_server *s); + +bool arch_handle_hvm_io_completion(enum hvm_io_completion io_completion); + +int hvm_get_ioreq_server_range_type(struct domain *d, + ioreq_t *p, + uint8_t *type, + uint64_t *addr); + +void arch_hvm_ioreq_init(struct domain *d); +void arch_hvm_ioreq_destroy(struct domain *d); + +#define IOREQ_IO_HANDLED X86EMUL_OKAY +#define IOREQ_IO_UNHANDLED X86EMUL_UNHANDLEABLE +#define IOREQ_IO_RETRY X86EMUL_RETRY + #endif /* __ASM_X86_HVM_IOREQ_H__ */ /*