Re: [RFC 1/1 linux-next] tools/lguest: update variables to avoid setreg(eax, eax)

[Rusty Russell <rusty@rustcorp.com.au>]

Fabian Frederick <fabf@skynet.be> writes:
> eax is used for offset calculation but is also a local
> variable used to store register value.

Well, it's the value we're going to put back in eax.  And it came from
eax.

Renaming val to def_val is weird, since it's not the default.  You can
tell it's set in various places.

I don't think this patch adds clarity.  What are you trying to do?

Thanks,
Rusty.

>  tools/lguest/lguest.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/tools/lguest/lguest.c b/tools/lguest/lguest.c
> index e440524..d466c79 100644
> --- a/tools/lguest/lguest.c
> +++ b/tools/lguest/lguest.c
> @@ -1611,12 +1611,12 @@ static void emulate_insn(const u8 insn[])
>  {
>  	unsigned long args[] = { LHREQ_TRAP, 13 };
>  	unsigned int insnlen = 0, in = 0, small_operand = 0, byte_access;
> -	unsigned int eax, port, mask;
> +	unsigned int val, port, mask;
>  	/*
>  	 * Default is to return all-ones on IO port reads, which traditionally
>  	 * means "there's nothing there".
>  	 */
> -	u32 val = 0xFFFFFFFF;
> +	u32 defval = 0xFFFFFFFF;
>  
>  	/*
>  	 * This must be the Guest kernel trying to do something, not userspace!
> @@ -1692,29 +1692,29 @@ static void emulate_insn(const u8 insn[])
>  	 * If it was an "IN" instruction, they expect the result to be read
>  	 * into %eax, so we change %eax.
>  	 */
> -	eax = getreg(eax);
> +	val = getreg(eax);
>  
>  	if (in) {
>  		/* This is the PS/2 keyboard status; 1 means ready for output */
>  		if (port == 0x64)
> -			val = 1;
> +			defval = 1;
>  		else if (is_pci_addr_port(port))
> -			pci_addr_ioread(port, mask, &val);
> +			pci_addr_ioread(port, mask, &defval);
>  		else if (is_pci_data_port(port))
> -			pci_data_ioread(port, mask, &val);
> +			pci_data_ioread(port, mask, &defval);
>  
>  		/* Clear the bits we're about to read */
> -		eax &= ~mask;
> -		/* Copy bits in from val. */
> -		eax |= val & mask;
> +		val &= ~mask;
> +		/* Copy bits in from defval. */
> +		val |= defval & mask;
>  		/* Now update the register. */
> -		setreg(eax, eax);
> +		setreg(eax, val);
>  	} else {
>  		if (is_pci_addr_port(port)) {
> -			if (!pci_addr_iowrite(port, mask, eax))
> +			if (!pci_addr_iowrite(port, mask, val))
>  				goto bad_io;
>  		} else if (is_pci_data_port(port)) {
> -			if (!pci_data_iowrite(port, mask, eax))
> +			if (!pci_data_iowrite(port, mask, val))
>  				goto bad_io;
>  		}
>  		/* There are many other ports, eg. CMOS clock, serial
> @@ -1722,7 +1722,7 @@ static void emulate_insn(const u8 insn[])
>  	}
>  
>  	verbose("IO %s of %x to %u: %#08x\n",
> -		in ? "IN" : "OUT", mask, port, eax);
> +		in ? "IN" : "OUT", mask, port, val);
>  skip_insn:
>  	/* Finally, we've "done" the instruction, so move past it. */
>  	setreg(eip, getreg(eip) + insnlen);
> -- 
> 2.4.2