[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support
|
On 2/22/2016 12:14 PM, Jan Beulich
wrote:
On 19.02.16 at 19:01, <czuzu@xxxxxxxxxxxxxxx> wrote:
On 2/19/2016 7:15 PM, Jan Beulich wrote:
On 19.02.16 at 17:25, <czuzu@xxxxxxxxxxxxxxx> wrote:
On 2/19/2016 4:26 PM, Jan Beulich wrote:
On 18.02.16 at 20:35, <czuzu@xxxxxxxxxxxxxxx> wrote:
---
MAINTAINERS | 1 +
xen/arch/arm/hvm.c | 8 +++
xen/arch/x86/hvm/event.c | 116 ++++++----------------------------------
xen/arch/x86/hvm/hvm.c | 1 +
xen/arch/x86/monitor.c | 14 -----
xen/arch/x86/vm_event.c | 1 +
xen/common/Makefile | 2 +-
xen/common/hvm/Makefile | 3 +-
xen/common/hvm/event.c | 96 +++++++++++++++++++++++++++++++++
So here you _again_ try to introduce something HVM-ish for ARM.
Why? Why can't this code live in common/vm_event.c?
Are you referring to "xen/arch/arm/hvm.c"? If so, I did not introduce
that file, it was already there, all I did is add handling of
HVMOP_guest_request_vm_event to ARM-side's already existing do_hvm_op :).
No, I'm referring to your initial attempt to create arch/arm/hvm/...
I don't understand. Have I done that again with this patch?
Not the exact same thing, but something going along those same
lines of thinking.
On the "HVM-ish" note, is there some incompatibility between ARM and the
concept of HVM?
ARM guests are neither PV nor HVM right now, but somewhere in
the middle (PVHv2 may come closest).
I did not know that, but the fact that there is already "hvm-like" code
written for ARM didn't hint me towards that fact either :)
I'm aware that I'm far from familiar with the codebase right now, I'm
browsing more of the code these days and taking notes to try and
understand in depth at least the parts I'm sending contributions for.
I've already got some questions I want to post to the mailing list soon,
*including* exactly how the distinction between the guest-types comes
into play w/ the vm-events code.
Specifically, I'm talking for example about the following piece of code
from the X86 arch_monitor_get_capabilities:
/*
* At the moment only Intel HVM domains are supported. However, event
* delivery could be extended to AMD and PV domains.
*/
if ( !is_hvm_domain(d) || !cpu_has_vmx )
return capabilities;
== "However, event delivery could be extended to AMD and PV domains".
This comment begs for questions like:
* what would be necessary to extend support to PV domains?
* can we really do this operation without hardware assisted
virtualization whatsoever? If not, how much can we do without that?
* what about pvh?
Since I have other questions like the above and I'll probably have more
while I'm trying to get a better picture of the code, would it be ok if
we defer addressing these issues to then?
Yes, you should definitely not hijack this thread for other, more
general inquiries.
Ok then, should I also understand then that for now it's ok to keep
the "HVM-ish" hvm_event_traps & hvm_event_guest_request (I
suppose you were referring to these 2 functions above) on the
common-side event.c until we address these issues?
Or I could try to move them to common/vm_event.c as you suggest
renamed to vm_event_traps & vm_event_guest_request and also
rename arch_hvm_event_fill_regs to arch_vm_event_fill_regs (?).
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -376,17 +376,15 @@ struct arch_domain
unsigned long *pirq_eoi_map;
unsigned long pirq_eoi_map_mfn;
- /* Monitor options */
+ /* Arch-specific monitor options */
struct {
- unsigned int write_ctrlreg_enabled : 4;
- unsigned int write_ctrlreg_sync : 4;
- unsigned int write_ctrlreg_onchangeonly : 4;
- unsigned int mov_to_msr_enabled : 1;
- unsigned int mov_to_msr_extended : 1;
- unsigned int singlestep_enabled : 1;
- unsigned int software_breakpoint_enabled : 1;
- unsigned int guest_request_enabled : 1;
- unsigned int guest_request_sync : 1;
+ uint16_t write_ctrlreg_enabled : 4;
+ uint16_t write_ctrlreg_sync : 4;
+ uint16_t write_ctrlreg_onchangeonly : 4;
+ uint16_t mov_to_msr_enabled : 1;
+ uint16_t mov_to_msr_extended : 1;
+ uint16_t singlestep_enabled : 1;
+ uint16_t software_breakpoint_enabled : 1;
} monitor;
What is this type change supposed to achieve in general, and in
particular in the context of this patch?
Some time before this patch there was a discussion I had w/ ARM
maintainer Ian Campbell who made the suggestion to try and move parts of
the monitor vm-events code to the common-side.
(you can find that discussion here:
https://www.mail-archive.com/xen-devel@xxxxxxxxxxxxx/msg54139.html).
In short, the reason for his suggestion was that that previous attempt
of mine to add guest-request support for ARM highlighted code
duplication between X86 and ARM if I were to leave the monitor vm-events
code as it was (that is, completely arch-specific). In an effort to
avoid that, I first submitted a 7 patch-series which tried to move as
much as possible to the common-side.
"As much as possible" meant guest-request, ctrl-reg write, single-step
and software breakpoints, all moved to the common-side. That also meant
introducing some Kconfigs to selectively activate some parts of that
now-common code, since not all of it was used on ARM at that moment.
Although conceptually that code could have remained on the common-side,
Tamas pointed out that we could get rid of the Kconfigs (which in his
opinion bloated the code) by only moving at the moment what is
implemented on both X86 and ARM. As for the rest, he noted that when I
add implementations for the other monitor vm-events on ARM as well, I
could move that to common as well. The above is a result of that -
guest-request is implemented on both X86 and ARM with this patch, hence
I also moved it to common.
I agree there are two fields being removed. But the question was
why you also change the type of the other fields.
It made sense because:
* we don't need 32-bits to store that data anymore
* as implementations are laid out for ARM also, more of those bits will
get moved to common, so even less bits will be needed
Altering bit field types from signed or unsigned int to some other
type always need more rational than that, because from a strict
language perspective only those two types can be used for bit
fields in a compatible manner. Furthermore, gcc had (and maybe
still has) some "interesting" (at least to someone having come
from a different world years ago) behavior for bit fields of types
wider than "int", which implicitly makes it desirable to avoid non-
standard types unless absolutely needed in a specific case. (And
such then shouldn't be mixed in with other changes, to make both
the adjustment and the reasons for them stand out.)
Jan
That's another reason for me to read the C standard someday.
(C90,
6.5.2.1) "A bit-field shall have a type that is a qualified or
unqualified version of one of int, unsigned int, or signed int"
(C99, 6.7.2.1p4) "A bit-field shall have a type that is a
qualified or unqualified version of _Bool, signed int, unsigned
int, or some other implementation-defined type."
I've always used bitfields w/ arbitrary sizes: char, short,
int, long long...GCC never has complained about it, I didn't know
types other than signed/unsigned int were non-standard extensions.
GCC warns me about this if I use "-std=gnu90 -pedantic", w/ gnu99 it
doesn't, maybe because of the "or some other
implementation-defined type" part (which I don't fully
understand).
Will change back the uint16_t to plain unsigned int and uint8_t
here:
+ /* Common monitor options */
+ struct {
+ uint8_t guest_request_enabled : 1;
+ uint8_t guest_request_sync : 1;
+ } monitor;
to unsigned int as well.
Thanks,
Corneliu.
|
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
