On 08.12.20 09:52, Paul Durrant wrote:
Hi Paul, Jan
On 30.11.2020 11:31, Oleksandr Tyshchenko wrote:
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -142,8 +142,8 @@ void hvmemul_cancel(struct vcpu *v)
{
struct hvm_vcpu_io *vio = &v->arch.hvm.hvm_io;
- vio->io_req.state = STATE_IOREQ_NONE;
- vio->io_completion = HVMIO_no_completion;
+ v->io.req.state = STATE_IOREQ_NONE;
+ v->io.completion = IO_no_completion;
vio->mmio_cache_count = 0;
vio->mmio_insn_bytes = 0;
vio->mmio_access = (struct npfec){};
@@ -159,7 +159,7 @@ static int hvmemul_do_io(
{
struct vcpu *curr = current;
struct domain *currd = curr->domain;
- struct hvm_vcpu_io *vio = &curr->arch.hvm.hvm_io;
+ struct vcpu_io *vio = &curr->io;
Taking just these two hunks: "vio" would now stand for two entirely
different things. I realize the name is applicable to both, but I
wonder if such naming isn't going to risk confusion.Despite being
relatively familiar with the involved code, I've been repeatedly
unsure what exactly "vio" covers, and needed to go back to the
Good comment... Agree that with the naming scheme in current patch the
code became a little bit confusing to read.
header. So together with the name possible adjustment mentioned
further down, maybe "vcpu_io" also wants it name changed, such that
the variable then also could sensibly be named (slightly)
differently? struct vcpu_io_state maybe? Or alternatively rename
variables of type struct hvm_vcpu_io * to hvio or hio? Otoh the
savings aren't very big for just ->io, so maybe better to stick to
the prior name with the prior type, and not introduce local
variables at all for the new field, like you already have it in the
former case?
I would much prefer the last suggestion which is "not introduce local
variables at all for the new field" (I admit I was thinking almost the
same, but haven't chosen this direction).
But I am OK with any suggestions here. Paul what do you think?
I personally don't think there is that much risk of confusion. If there is a
desire to disambiguate though, I would go the route of naming hvm_vcpu_io
locals 'hvio'.
Well, I assume I should rename all hvm_vcpu_io locals in the code to
"hvio" (even if this patch didn't touch these places so far since no new
vcpu_io fields were involved).
I am OK, although expecting a lot of places which need touching here...
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -145,6 +145,21 @@ void evtchn_destroy_final(struct domain *d); /* from
complete_domain_destroy
*/
struct waitqueue_vcpu;
+enum io_completion {
+ IO_no_completion,
+ IO_mmio_completion,
+ IO_pio_completion,
+#ifdef CONFIG_X86
+ IO_realmode_completion,
+#endif
+};
I'm not entirely happy with io_ / IO_ here - they seem a little
too generic. How about ioreq_ / IOREQ_ respectively?
I am OK, would like to hear Paul's opinion on both questions.
No, I think the 'IO_' prefix is better. They relate to a field in the vcpu_io
struct. An alternative might be 'VIO_'...
+struct vcpu_io {
+ /* I/O request in flight to device model. */
+ enum io_completion completion;
... in which case, you could also name the enum 'vio_completion'.
Paul
ok, will follow new renaming scheme IO_ -> VIO_ (io_ -> vio_).
--
Regards,
Oleksandr Tyshchenko
